Design comparison
Solution retrospective
This is my first challenge so I need your feedback, Thank youπ
Community feedback
- @VCaramesPosted almost 2 years ago
Hey there! π Here are some suggestions to help improve your code:
Congrats on completing your first challenge!ππ
- Since the images in this component add value and serve a purpose (displaying the product), it is best to use the
Picture
element and not thebackground-image
property, as it will to use different images during different breakpoints.
Here is an example of how it works: EXAMPLE
Syntax:
<picture> <source media="(min-width: )" srcset=""> <img src="" alt=""> </picture>
More Info:π
https://www.w3schools.com/html/html_images_picture.asp
- The name of the perfume , βGabrielle Essence Eau De Parfumβ, is the most important content in your card so it should be wrapped in a heading Element.
- Currently, the old price (169.99) π· is not being properly announced to screen readers. To fix this, you are going to wrap the the price in a
del
element and inside it you will add aspan
element with ansr-only class
that will state something like βThe previous price wasβ¦β and use CSS to make it only visible to screen readers.
More Info:π
- Implement a Mobile First approach π± > π₯
With mobile devices being the predominant way that people view websites/content. It is more crucial than ever to ensure that your website/content looks presentable on all mobile devices. To achieve this, you start building your website/content for smaller screen first and then adjust your content for larger screens.
More Info:π
- Your CSS Reset is extremely bare and being underutilized. To fully maximize your CSS reset, you want to add more to it.
Here are few CSS Resets that you can look at and use to create your own or just copy and paste one that is already prebuilt.
https://www.joshwcomeau.com/css/custom-css-reset/
https://meyerweb.com/eric/tools/css/reset/
http://html5doctor.com/html-5-reset-stylesheet/
If you have any questions or need further clarification, feel free to reach out to me.
Happy Coding!ππ
Marked as helpful1@HazemHussein14Posted almost 2 years ago@vcarames Thank you for this valuable information about the picture element and of course for the interesting resources and techniquesπ π
0 - Since the images in this component add value and serve a purpose (displaying the product), it is best to use the
- @HassiaiPosted almost 2 years ago
Replace <div class="container"> with the main tag, <p class="product-name"> with <h1> and <div class="attribution"> with the footer tag to fix the accessibility issues. for more on semantic html visit https://web.dev/learn/html/semantic-html/
To center .container on the page, add min-height:100vh; display: flex; align-items: center: justify-content: center; or min-height:100vh; display: grid place-items: center to the body. and replace
margin: 200px auto;
withmargin: 0 auto;
Use rem or em as unit for the padding, margin, width and preferably rem for the font-size for more on this watch this https://youtu.be/N5wpD9Ov_To
Hope am helpful.
HAPPY CODINGMarked as helpful1@HazemHussein14Posted almost 2 years ago@Hassiai thanks for your feedback it was really helpful
0 - @Abhirocks889Posted almost 2 years ago
Hello Hazem, first of all congratulations on submitting your first challenge. I am sure this is the beginning of many more to come.
Regarding the solution, here are some of my pointers:
- The design looks great. I particularly like the addition of the attribution.
- The solution is responsive too. So, that is wonderful. However, for widths <= 600px, you could consider adding some padding to the body so that the card always has some space around it. Something like:
@media (max-width: 600px) { body { padding: 20px; } }
The other main feedback is regarding semantic HTML. Generally, it is considered a better practice to use semantic elements like (section, article, header, etc.) instead of plain
div
andspan
. This is to improve accessibility. You can read more about it here: Importance of Semantic HTMLAgain, congratulations on your first solution and keep learning and growing.
Regards, Abhishek
Marked as helpful1@HazemHussein14Posted almost 2 years ago@Abhirocks889 Thank you I will modify the code it was a nice idea
1
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord