Single price component (7-1 pattern/SASS)

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 3 years ago

  👋Hi Said Alejandro!

  It looks great 🙌 on both portrait and landscape on my mobile device.

  Now for the feedback on this solution:

  • I would recommend to use ul and li instead of p and span for the list.
  • Also keep the default HTML font size 100%. If you cut it off to 62.5% it will make the users who already set bigger font size will still see small font size while for the opposite, they will see very small text (like me).
  • For the body font size, you need to use rem unit too.
  • I noticed that you used margin-block, padding-block, and other logical CSS properties. In my opinion, it would be much better if you use the margin and padding property or more "common" CSS property, since they 100% get supported by all browsers. But, if you may want to use them, you might need to use @support to support older browser.

  That's it! Hopefully this is helpful!

  Marked as helpful

  2

  ---

  

  Said Alejandro Rosas Vera•145

  @said-alrovePosted over 3 years ago

  @vanzasetia ✌ Hi!, first of all, thanks for your feedback, and I'd like to reply to everything:

  ✅.- Your first recommendation is already done 👏, I had already done this challenge a couple of months ago when I wasn't too much confident with CSS and HTML, therefore I guess using spans instead of a list made more sense for me, thus when recreating it I was trying to focus on recreating a cleaner code (with the 7-1 patter) more than checking if the original code was correct, so ... my bad 😥.

  ✅.- Using 16px in the body to reset the root size to its original was one of those things I learned when I started with CSS, therefore I had never asked myself if that was totally correct, I just assumed it, but now that you mention that, using 1.6rem makes more sense (plus I looked for information after reading your comment for a fact check and yep, you're right), so ... thank you!.

  ✅.- Yeah, I know, but I use them basically because these are simple challenges that maybe nobody else out of the people from here is gonna see (I'd like to use these challenges in my portfolio in the future, but because logical properties will be good enough supported when I finish of creating it, or at least I hope so, nevermind either), therefore I'd like to get used to using them to be prepare when those properties would be something as normal as the common ones.

  Yep, your comment was very helpful 😸, thank you so much for taking a bit of your time to write it!.

  Have a nice day :D!.

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 3 years ago

  @said-alrove 👍Great job on improving your code!

  Your second point about root font size, I was talking about the html font size.

  What I meant that, by changing the root font size, which meant the html font size.

  html {
    font-size: 62.5%;
  }
  

  It will make the users who already set bigger font size will still see small font size while for the opposite, they will see very small text (like me).

  So, for example if user set their browser font size to 20px, they will see 12.5px and for the one who set it to 14px, they will see 8.75px (too small).

  The math:

  user font size * 67.5% = what they will see.
  

  I recommend to keep the root font size 100% (the default).

  😀 Hopefully this clarified what I said before.

  0

  

  Said Alejandro Rosas Vera•145

  @said-alrovePosted over 3 years ago

  @vanzasetia Yep, now I get the point, I took it for granted 😥, I'll continue looking for more information for things like these, and I'll apply them in future projects 😸.

  0
• 

  P

  Patrick•14,265

  @palgrammingPosted over 3 years ago

  You should put a :hover state on your button

  Marked as helpful

  1

  ---

  

  Said Alejandro Rosas Vera•145

  @said-alrovePosted over 3 years ago

  @palgramming Done ✅, thank you :D!.

  0