Order Summary Component using CSS Grid

Community feedback

• 

  Elaine•11,400

  @elaineleungPosted about 2 years ago

  Hi Vincent Daniel, first off, well done in building this solution, which looks great on the whole. Just wanted to address the points you made in your note:

  About clamp(), I used to be quite confused about it and didn't find it useful, that is until I came across this CSS-tricks article on how clamp() works. There's a bit of math, but I think the explanation is really clear on how it can be used, and most importantly, they provided a little tool where you just need to input the min and max screen sizes and well as min and max size of the element. I have this bookmarked and use it every time I need clamp(), and it has made things so much easier for me.

  About min(), you can try width: min(100% - 2rem, 19rem) in your mobile view. What this means is that the browser will either choose 100% - 2rem (the 2rem is for the left and right margin around the component at 1 rem each), or it will choose 19rem, depending on which one is smaller. The only other thing you'd have to do is to add some padding in .card, remove the side padding in the button and give it a width of 100%.

  I encourage you to keep experimenting with min(), as this is a really powerful and useful tool, one that I use in every single project ever since I learned about it from Kevin Powell. Good luck!

  Marked as helpful

  1

  ---

  

  Vincent Daniel•410

  @davinceeyPosted about 2 years ago

  @elaineleung thank you soo much again for this feedback! I actually had some other questions for you after you reviewed my nft card component of which I learnt greatly from as I went ahead to Kevin Powell's tutorials. If there is a way of contacting you, I would be grateful. Thank you once again!

  1

  

  Elaine•11,400

  @elaineleungPosted about 2 years ago

  @davinceey No problem at all Vincent, glad to help! You can DM me on Twitter if you have it (link should be in my profile), or you can also find me on Slack in the FEM workgroup.

  Marked as helpful

  1
• 

  Lucas 👾•104,340

  @correlucasPosted about 2 years ago

  👾Hello Vincent Daniel, Congratulations on completing this challenge!

  Great start and great first solution! You’ve done really good work here putting everything together, I’ve some suggestions you can consider applying to your code:

  1.I saw that for some properties you’ve used rem and for others px. In this case it is better to use only one kind of unit to have a better organization for your code. relative units as rem or em that have a better fit if you want your site more accessible between different screen sizes and devices. REM and EM does not just apply to font size, but to all sizes as well.

  2.Use a CSS reset to avoid all the problems you can have with the default CSS setup, removing all margins, making the images easier to work, see the article below where you can copy and paste this css code cheatsheet: https://piccalil.li/blog/a-modern-css-reset/

  ✌️ I hope this helps you and happy coding!

  Marked as helpful

  0

  ---

  

  Vincent Daniel•410

  @davinceeyPosted about 2 years ago

  @correlucas thank you so much for your feedback! Wow. I feel so delighted seeing this and would improve in my usage of responsive units. Thank youu!

  0
• 

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  Hi Vincent, congratulations on completing the challenge, great job! 😁

  clamp() I think it is useful above all with the size of the texts but not always

  Some little tips for optimizing your code:

  • To fix the top image in the background just put more specific background properties to the body:

  background: url("../img/pattern-background-desktop.svg") no-repeat top center;
  background-size: contain;
  background-color: #e0e8ff;
  

  • add main tag and wrap the card for improve the Accessibility
  • also you can use article tag instead of a simple div to the container card for improve the Accessibility
  • add descriptive text in the alt attribute of the images
  • add transition on the element with hover effect
  • instead of using px use relative units of measurement like rem -> read here

  Hope this help! Happy coding 😉

  Marked as helpful

  0

  ---

  

  Vincent Daniel•410

  @davinceeyPosted about 2 years ago

  @denielden I appreciate this... I really anticipated a feedback on this. I would go ahead to improve my code. Thank You!

  1

  

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  @davinceey You are welcome! Keep it up :)

  0