Product Preview Card / Sass

Community feedback

• 

  Rajesh Janyani•330

  @Rajesh7rjPosted over 2 years ago

  @ChrisAndrewsDev Good effort on first one. Here I wanna help you on you 3 ACCESSIBILITY ISSUES.

  Document should have one main landmark, this encloses the main part of your project, you can fix it like this:

  <div class="homeWrapper"> <-- remove <main class="homeWrapper"> --> add

  All page content should be contained by landmarks, at the end of your code you have a div that can easily be a footer:

  <div class="attribution"> <-- remove <footer class="attribution"> --> add

  I hope this will help you.

  Good wishes......

  keep coding..... keep learning.......

  Marked as helpful

  1

  ---

  

  Chris Andrews•170

  @ChrisAndrewsDevPosted over 2 years ago

  @Rajesh7rj Thanks for the feedback! I will adjust that now :)

  0
• 

  Pradeep Saini•990

  @pradeeps4iniPosted over 2 years ago

  Hi, Chris. How are you?

  Congrats on implementing the design.

  I'd like to suggest some changes. I think they would make the code and ui look better.

  1. On the <contentWrapper> element. You are using "width:40%;". Which makes the content shrink too much when we reduce the wiewport width. You can use following code to make it look better:

     `max-width: 500px;

     margin-inline: auto;

     padding-inline: 1rem; `

  2. You should change the mobile layout breakpoint from 375px to 450px. Content of the card shrinks too much between 375px-400px;

  3. You can create an utility class ".flex {display: flex}". And use, class="flex", on elements where you want to use flexbox display.

  1

  ---

  

  Chris Andrews•170

  @ChrisAndrewsDevPosted over 2 years ago

  @pradeeps4ini I'm quite well thankyou!

  I'm running into some issues with that code suggestion, it removes the border radius and seems to squish the container, any clue why that would happen?

  Good call on the utility class!

  Thankyou for the feedback, it's much appreciated!

  0

  

  Pradeep Saini•990

  @pradeeps4iniPosted over 2 years ago

  @ChrisAndrewsDev

  1. Border-radius is working but because of padding-inline, it's not visible. You can use outline property to see it. I can't figure out why it's happening this way. I'll get back to you, once i figure it out.

  2. Container was squishing because the content needed more than 500px. So, 600px is the max-width we need.

  Why are you not using images in <img>. You can use <picture> element to change the image for different screen width sizes. Why use it as CSS bg image?

  1

  

  Chris Andrews•170

  @ChrisAndrewsDevPosted over 2 years ago

  @pradeeps4ini Oh, I've not used the picture element before! That seems quite handy, I will check it out!

  0