@Miguelaeb
Posted
Hi @Laila-Haddad, congratulations on your solution!
Great solution and a great start! From what I saw you’re on the right track. I’ve a few suggestions for you that you can consider adding to your code:
Use a CSS reset to avoid all the problems you can have with the default CSS setup, removing all margins, and making the images easier to work, see the link below where you can download this file which contains the normalize.css: https://necolas.github.io/normalize.css/
Use <main> instead of a simple <div> this way you improve the semantics and accessibility showing which is the main block of content on this page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.
Replace the <h2> containing the main title with <h1> note that this title is the main heading for this page and every page needs one h1 to show which is the most important heading. Use the sequence h1 h2 h3 h4 h5 to show the hierarchy of your titles in the level of importance, never jump a level.
Add a margin of around margin: 20px to avoid the card touching the screen edges while it scales down.
Use relative units as rem or em instead of px to improve your performance by resizing fonts between different screens and devices. These units are better to make your website more accessible. REM does not just apply to font size, but to all sizes as well.
You should have put the CSS code in an external file, that would have made the code easier to manage.
Here is an example:
https://www.youtube.com/watch?v=WuvujA5facU
Also, you should have added a min-width of 100vh to the container in order to centralize the main box.
Here's my solution for this challenge if you want to see how I build it: https://www.frontendmentor.io/solutions/product-preview-card-component-MCflXO_TZz
I hope this helps you and happy coding!
Marked as helpful
@Laila-Haddad
Posted
@Miguelaeb hey Miguel, you have no idea how much i appreciate your feedback. after going over your suggestions (and agreeing with almost every single one of them), i made some edits on my code. you can check the newly updated result above if you're interested .
I will say comparing your code to mine helped me a bunch in understanding how to vertically centralize the card, but i still don't quite get how the min-height :100vh; worked. I know that vh stands for viewport height, so does that mean i'm setting the container height to be (at least) a 100% of the screen height?
And again thank you so much for your time and effort. I hope you're having a wonderful day!
@Miguelaeb
Posted
@Laila-Haddad Hey Laila I'm glad my suggestions helped you.
Min height 100vh means the element should occupy the web browser viewport height. This is always 100 percent of the web browser's viewport height. If there is more content, the element will stretch more than the viewport's height.
