Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Responsive flex layout card

@ayobanjo

Desktop design screenshot for the Product preview card component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


I think my media query is a bit shaky when going from desktop to mobile. There were spill outs. How can I handle this better?

Community feedback

T
Grace 29,310

@grace-snow

Posted

Hi

The preview site link isn't working on this? I can give feedback but it's waaaay easier if I can view in browser

With HTML:

  • You've commented out the picture element. Why? That's exactly what you should be using here
  • Perfume should be in a paragraph tag and have letter spacing applied in css, not extra spaces in html
  • This card only has one heading element - the name of the perfume. Headings are the most important elements for giving your web document semantic structure (like any document, or a book with contents page - that's what headings are for). It's really important you don't misuse or misorder heading elements
  • the new price is not a heading
  • the old price needs wrapping in a <del> tag and some sr-only text adding before it to make it clear it is the old price, as screen readers are not told when text is styled with a line through it.
  • The attribution should be in a footer element

With the CSS

  • Font size should never be in px - use rem
  • remove max width 1440px
  • did you mean to add a min-height of 100vh to the body? That's what will center it for you on desktop screens
  • Not sure why you have padding on the body and margin on main and 90% width on something (?) You only need one of these techniques if the purpose is to prevent the component hitting screen edges on smaller screens
  • Try to name classes better. _1 and _2 etc will be hard for your future self to understand
  • Min-width 376px is really really small to make a switch to the desktop layout! You should only be switching layout when there is room for it to change.

Marked as helpful

0

@ayobanjo

Posted

@grace-snow Thank you very much for your response. Had a little bit of an issue on GitHub with the deployment when I tried to restructure the project folder. Its already working well now

0

@ayobanjo

Posted

@grace-snow Applied all your corrections. Thank you very much

0
Hyron 5,870

@hyrongennike

Posted

Hi @ayobanjo,

Nice job on your first attempt of the challenge

You can remove the max-width: 1440px on the body by doing this the card will be in the middle of the page.

The below is for mobile and will the stack the image and content.

main.main > div {
    width: 100%;
}

main.main {
    flex-direction: column;
}

Hope this is helpful.

Marked as helpful

0
Lea 160

@Hatchino

Posted

Hi. First, the r of "perfume" is missing. It also lacks the letter-spacing property which allows you the spacing between lines for your paragraph. The icon is not the same as on the model. Your responsive is not adjusted between 440px and 374px (the price exceeds the card). Adding styling to the attribution is just my opinion.

Marked as helpful

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join 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