Responsiveness: Does the header behave as expected on different screen sizes? Is the mobile menu toggle clear?
Styling: How does the color scheme and styling resonate with the overall design of the website?
Usability: Is the user experience smooth when interacting with the header and the mobile menu?
If you think something can be more good you can just tell me
Hi, you did a great job! A few things that jumped out to me at a quick glance...
It looks like some of the fonts (fraunces) don't match. The headers and buttons, some links.
When you open the mobile menu, it pushes the content below it down. Try making the mobile menu position: fixed or absolute. That way it won't affect the elements around it. You would then need to adjust some of the mobile menu styles, such as width and you can position the menu using top, left, etc. Also, a small but important thing that I think a lot of people forget is adding cursor:pointer to the hamburger menu to tell the user it's a clickable element.
The menu links don't seem to go anywhere. You can add an id to each section and then link to the id. For example, the section below the hero could have an id="aboutSection", then in the menu, the a tag would be, <a href="#aboutSection">About</a>. You could do this in the footer menu as well. When doing this, you also want to be sure to close the mobile menu when you click a link, the same way you would when clicking the hamburger. While you're at it, you might want to change the sections from <div> to <section>.
The client testimonials section looks a bit crammed when it first changes from column to row. Maybe try a different, larger breakpoint, so it doesn't change to row until a larger screen size that looks a little less crammed. Also, maybe adjust the padding and margin at smaller screen sizes, which would give it more room to "breathe"
Hope those tips are helpful. Happy coding!
I have a feeling my solution is not the most elegant when it comes to the CSS styling for the mobile design. I had to "tweek" a few things in the product div to get it to be the correct height and background color.
I would love any suggestions on how to get better at making mobile-friendly designs. I'm open to specific suggestions to this project or general resources. Thanks.
Hey Anthony,
Good job!
For your comment about mobile friendly designs, it's highly recommended to use a mobile first approach and then add media queries to change things up at larger screen sizes when needed. If you go to youtube and search ''kevin powell mobile first', the first video that pops up is really helpful. That guy is a great CSS teacher in general.
Some things I noticed...
It looks like you used height in a lot of places when you really don't need it. You have a height on the container and the product image divs. In dev tools, I went in and removed all of the heights and everything looks pretty much the same. Often, you don't need to specify heights, you can let the content dictate the height.
I noticed you used flex-box more than necessary. I used to do this a lot. Once again, in dev tools, I removed display: flex (and other flex related properties) from .product-image and .product-description and things looked the same, so generally you can avoid writing excess code.
I noticed for the word perfume, you have it capitalized and you added extra spaces in the html. You should avoid manually adding spaces. Instead, in your CSS, you can use letter-spacing to control exactly how much space you want between each letter and you can use text-transform: uppercase to change the casing
Finally, you can avoid specifying grid-template-rows in this case. Since your grid container has only 2 children (.product-image and .product), when you set the grid-template-columns to 1fr, it automatically will add the second row.
I hope these suggestions help. Happy coding!
This is my solution to this challenge, please help me to improve my skills. Comments and suggestions are welcome. Thank you!
Hi Ibrahim,
Good job! A few things I noticed that might help. For your grid, I think you might be over complicating it a bit, which is something I used to do myself. I noticed that you made your grid-template-columns: repeat (12, 1fr), and then each "box" spans multiple columns, but I recommend just setting the columns to the exact amount that you need. For example, on mobile, you can literally make your grid-template-columns: 1fr and then set a media query for desktop and change it to grid-template-columns: repeat (4, 1fr). That way you should be able to avoid setting the grid children to span columns and rows. They should automatically fall into place.
Another thing I would recommend is avoiding making the images in the grid background images. If you just make them images with a width and height of 100% and then add object-fit:cover, you should be able to avoid setting a specific height to your boxes. If you want to take it a step further, you can use the <picture> tag and set the srcset according to screensize to choose the "mobile" version at smaller screen sizes and the "desktop" at larger sizes.
Hopefully, those suggestions help. Happy coding!
Hey everybody, I would appreciate any feedback. There's a few things that I couldn't quite figure out.
How to change the color of the SVG images. In dark mode, they're supposed to be white. Also, when something is 'Not Available' (i.e. twitter handle) it's supposed to be greyed out. I can't figure out how to change the color at all, let alone how to programmatically change it when something is 'Not Available'.
Another thing that's driving me crazy is that when the screen width gets too small the button gets pushed out of the form. Even worse, when the 'No result' error is there, it pushes it even further and 'No result' goes to 2 lines, for some reason.
Also, I feel like I could definitely refactor some of my javascript. This is my first "junior" project, after doing a few "newbies".
Thanks in advance for any help!
One more thing I forgot to ask about is the 'joined' date. The API gives it to you in the ISO format, but the design has the month as "Jan" instead of 01, for example. Anyone know how to convert the month to text like that? Thanks again!
Hey guys,
This is my second submission. Made a few suggested changes in the css and am happy with the layout for the most part. Still would like to improve the functionality. I want to be able to close the accordion when clicking on the question that's already open. As of now, you can open the accordion when clicking on a question and it closes when you click on and open another, but I would like to be able to click the open one again to close it. Any suggestions? Thanks!
Awesome! Can't wait to implement these changes tomorrow and see how it turns out! Thanks so much for the thorough tips and recommendations!
Hi Everyone,
I had a blast learning to build an accordion with JavaScript for this challenge!
I have a couple of questions for anyone who might be able to help:
Thank you. Any feedback is really appreciated!
Brendan
Hi Brendan,
I'm still working on this challenge myself, but regarding your first question, I had a similar issue. Try using percentages instead of rems in transform(translate). Hope that helps.
