Design comparison
Solution retrospective
This is my first solution to try in this platform I'm beginner in web dev I would like to know if I did it according to the challenge, I'll appreciate your feedback.
Community feedback
- @MelvinAguilarPosted about 2 years ago
Hi @MarwaanAbdalla π, good job completing this challenge, and welcome to the Frontend Mentor Community! π
I have some suggestions you might consider to improve your code:
- Use the
<main>
tag to wrap all the main content in your solution instead of using<div class="container">
.
- Instead of using pixels in font size, use relative units of measure like
rem
orem
. The font size in absolute length units (px) does not allow users with limited vision to change the text size in some browsers. Reference.
- The
<br>
tag is not a semantic tag, so you should not use it. Also, if a screen-reader reads the text, it will break the flow of reading at the line break tag, so it should not be used to add vertical spacing. There are only a few cases where it is necessary (for example, in a poem or an address), and it is possible to avoid them by applying padding and margin styles via CSS. More information here.
- You can use a <picture> tag when you need to change an image in different viewports. Using this tag will prevent the browser from loading both images, saving bandwidth and preventing you from utilizing a media query to modify the image.
Example:
<picture> <source media="(max-width: 600px)" srcset="./images/image-product-mobile.jpg"> <img src="./images/image-product-desktop.jpg" alt="your_alt_text"> </picture>
I hope those tips will help you! π
Good job, and happy coding! π
Marked as helpful2@MarwaanAbdallaPosted about 2 years ago@MelvinAguilar Thanks for the help I would like to ask after changing from container to main tag the content of the page moved to left instead of center here is the updated code: https://github.com/shaalle/Product-preview-card-component
1@MelvinAguilarPosted about 2 years ago@MarwaanAbdalla You are using the class selector, but the
<main>
element does not have a class named main.main { . . . }
Update the selector to
main { . . . }
Or add class "main" in <main> element
<main class="main">
, I recommend the first optionMarked as helpful1@MarwaanAbdallaPosted about 2 years ago@MelvinAguilar Fixed it now thank you very much.
1 - Use the
- @VCaramesPosted about 2 years ago
Hey there! π Here are some suggestions to help improve your code:
-
To identify the main content of you site you will want to encase your entire component inside a Main Element.
-
The Alt Tag Description for the image needs to be improved upon. You want to describe what the image is; they need to be readable. Assume youβre describing the image to someone.
-
This challenges requires the use of two images π for different breakpoints. The Picture Element will facilitate this.
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
https://web.dev/learn/design/picture-element/
- The old price π· is not being announced properly to screen readers. You want to wrap it in a Del Element and include span element with an sr-only text explaining that this is the old price.
If you have any questions or need further clarification, let me know.
Happy Coding! π»π
Marked as helpful1 -
- @mdabdulrahmanPosted about 2 years ago
Hi @MarwaanAbdalla,
Welcome to Frontend mentor community, Your first solution is awesome and keep it up.
Some advices 1.Wrap all the content in the
<main>
tag so you couldn't get an landmark error.2.Use the alt attributes for images
Marked as helpful1
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