@steventoben
Posted
This looks pretty great tbh. I only have a couple small critiques which is rare for me when reviewing other peoples' stylesheets. First off I want to say that I love your use of rem units throughout the project and actually using them properly. I also love the inter-activeness from all of the buttons, however I feel like the textbox doesn't reciprocate the same feeling. I know it's a small thing, but even changing the cursor from a pointer to a caret once it has focus, and maybe even throwing an effect on the placeholder text would make a big difference. Another tiny detail that's purely semantics is you shouldn't skip a header level in html. It should go from h1 -> h2, which again isn't really a big deal especially with the fantastic accessibility of the rest of your markup. Your SASS file is pretty big, for a small project like this it's not a huge deal but a few more small improvements you could make would be splitting your variables into a partial, it also seems like you used some styles enough(ex: flexbox) to warrant breaking them off into mixins, perhaps into into another partial with your media query mixins? Another thing is in your last media query I think you nested to deep. For example, your button and input share the same exact set of rules. And here's the last little nitpick: are you using a typographic scale? A lot of your font-sizes are quite weird and stray from a typical modular type scale and I'm curious how you came to those numbers. Also you also never include line-height or letter-spacing into any of your text. I honestly had to dig pretty hard to find anything wrong with this, overall it looks pretty amazing and no one would ever really notice such small details. Great work!!
@Nafsuki
Posted
@steventoben Wow, thank you so much for such detailed feedback!!! I submitted other challenges & some didn’t get any feedback, which made me sad and when I get a great feedback, I can really learn a lot from that so I’m so thankful!! It means a lot & encourages me a lot!! ☺️
//* I also love the inter-activeness from all of the buttons, however I feel like the textbox doesn't reciprocate the same feeling. I know it's a small thing, but even changing the cursor from a pointer to a caret once it has focus, and maybe even throwing an effect on the placeholder text would make a big difference. *//
---You’re right! I didn’t know how to add effect on input so I skipped it, but since you gave me this feedback, I learned how to do it and added hover & focus effect on input. Hope you like it! Thank you so much for your feedback, without it I wouldn’t have taken it further to focus on this now to learn how to do it🙏
//* Another tiny detail that's purely semantics is you shouldn't skip a header level in html. It should go from h1 -> h2, which again isn't really a big deal especially with the fantastic accessibility of the rest of your markup. *//
---Oh, I didn’t really think about that…😅 I’m gonna stick to the order from now on! It was h1 -> h3 so I changed it to h1 -> h2 😄
//* Your SASS file is pretty big, for a small project like this it's not a huge deal but a few more small improvements you could make would be splitting your variables into a partial, it also seems like you used some styles enough(ex: flexbox) to warrant breaking them off into mixins, perhaps into into another partial with your media query mixins? Another thing is in your last media query I think you nested to deep. For example, your button and input share the same exact set of rules. *//
---Following your suggestions, I learned how to use mixins:)) Wow, it is very useful when I have duplicated styles. I fixed my code with mixins. It is my first time using it so hope I’m using it right. I also tried media query mixins in a separate file(partial?) for the first time. I hope I’m getting what you meant correctly.
//* And here's the last little nitpick: are you using a typographic scale? A lot of your font-sizes are quite weird and stray from a typical modular type scale and I'm curious how you came to those numbers. Also you also never include line-height or letter-spacing into any of your text. I honestly had to dig pretty hard to find anything wrong with this, overall it looks pretty amazing and no one would ever really notice such small details. Great work!! *//
---Oh, I didn’t know my font-sizes are weird.. 🙀 I placed h1-h6 in html randomly after how it looks on the design, and place main p font-size after the style guide(which was 20px this time) and adjusting other font sizes(again randomly..) by looking at the original design in css, and that’s how those numbers were come up.. Next time, I will stick to the typographic scale(you mean stick to h1-h6 original sizes?) :))!
If I’m understanding wrong, please be kind & let me know:)) Thank you so much for taking your time & giving me such detailed feedback!! I learned a lot from your advice and it gave me a great challenge to learn more & try to improve my codes!! I’m learning React now and would love to use it for challenges later. Hope you would come back & would review my code again soon!!! 😄
