My first mobile-focused page. Used my current love: Flexbox

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted about 2 years ago

  Hi Kathryn

  Here are some tips that should help you 😊

  • make the component main and the attribution footer. There is no need for a separate main div although it’s doing no harm
  • give the product image a better alt description. That’s an important bit of content so needs a proper description. Alt should be a human readable description that would allow someone to picture the image even if they can’t see it
  • the cart icon on the other hand is decorative so alt should be left blank on that.
  • remove the h1 from the price. Use paragraph there. You should only have one h1 per page. And only use headings when they are actually a title for some other content. Headings are the most important feature in html for giving content structure. Think about them like a contents page on a long document - each heading has to be in order to communicate the structure of content
  • the strike through text should be wrapped in a del element. Screenreader users are not notified about text that has a line through, so this also needs a span with an sr-only class on it and writing to inform users that’s the old price (otherwise they would just hear two prices read out which would be confusing)
  • you don’t need role of main on the main element. It already has that role built in.
  • remove height from the button. Instead use padding in a unit like em (or rem). This will ensure the button stays responsive even if a user changes the text sizes
  • never ever have font size in px. Use rem. Overall I think you are using px too much and would be better using rem more in other places too
  • padding is for internal spacings (like inside a box) and margin is for external spacing (like creating space between paragraphs). Often (not always) margin will only go in one direction but padding will be on all sides.
  • If you ever find yourself having to use !important it is highly likely there is something wrong elsewhere in your css. That indicates too high specificity elsewhere, so go fix it there. Try to avoid important at all costs
  • usually a css reset at the start of a stylesheet sets img tags to display block and max-width or width 100%
  • consider using the object fit property on the main product img, along with height 100%
  • line height is usually unitless, like 1.5, or occasionally in %, not in rem. not wrong, just unusual. It must never ever be in px though
  • letter spacing is usually in em so it scales with the current font size
  • I wouldn’t expect any max-width media query on this. If you are working mobile first, mobile styles are the base and there is no need for a media query to target that size
  • there is no need for a tablet and a desktop media query on this. All that needs to change on larger screens (whenever there is room for the side by side layout) is: max width of the component, flex direction, width/flex properties of the two halves

  Marked as helpful

  1
• 

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  👾Hello Kathryn, congratulations for your new solution!

  You did a good work putting everything together in this challenge, something you can do to improve the image that needs to change between mobile and desktop is to use <picture> instead of <img> wrapped in a div. You can manage both images inside the <picture> tag and use the html to code to set when the images should change setting the device max-width depending of the device (phone / computer)

  Here’s a guide about how to use picture: https://www.w3schools.com/tags/tag_picture.asp

  Note that you pricing section after 280px starts to go out the container, to fix that you need a media query to make the content stay in different row:

  @media (max-width: 320px) {
  .pricing {
      display: flex;
      flex-direction: column;
  }
  }
  

  👋 I hope this helps you and happy coding!

  Marked as helpful

  1
• 

  Elaine•11,400

  @elaineleungPosted about 2 years ago

  Hi Kathryn, great work on this first challenge, and welcome to Frontend Mentor! I saw your comment in another solution and decided to check out yours here, which I think was quite well done on the whole 🙂

  There are only two small suggestions I have:

  1. For the wrapper, I see that you're using flexbox for centering your component, which is great. What you can try next is, use min-height: 100vh instead of height: 100% so that there won't be scrolling needed inside the containers. To see what the difference, try shrinking the height of the browser and then toggle between the two declarations. I'd also suggest having some top/bottom margin for the component in case the screen gets too small for the content.

  2. In the desktop view, you can use flex to make the two columns equal. I see that you got a 50% width for the image; instead of using that, try flex: 1 0 50%.

  Hope to see more work, and keep going 😊

  Marked as helpful

  1

  ---

  

  Kathryn Bousquet•10

  @kbousquetPosted about 2 years ago

  @elaineleung I typically like using vh and vw, but I've heard they can be finicky. Have you ever experienced an issue with using it?

  And I had no idea I could set columns with flexbox like that! Thanks for the tips!

  0

  

  Elaine•11,400

  @elaineleungPosted about 2 years ago

  @kbousquet Oh you're welcome! 🙂 Regarding viewport units, I agree about them being finicky, and I rarely use them other than the 100vh min height on the body selector. I recently needed to use vw to turn something into a responsive square, and even for that I question whether there's a better way of doing it. I see people using them for heights and widths (even padding and margins), and I find that things don't tend to look optimal for all browser views. I don't use them mainly because I like using responsive properties instead such as width: min() and clamp(), and also I have better control with them than with viewport units.

  1

  

  T

  Grace•31,230

  @grace-snowPosted about 2 years ago

  @elaineleung you meant min-height 100vh there, right? (Not max)

  2

  

  Elaine•11,400

  @elaineleungPosted about 2 years ago

  @grace-snow That's right, thanks for catching that, Grace! Just corrected it 🙂

  2