Ping Coming Soon Page using Sass & JS

Community feedback

• 

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  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!!

  1

  ---

  

  Nafsuki•245

  @NafsukiPosted almost 4 years ago

  @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!!! 😄

  0
• 

  Gerben Dol•3,115

  @GerbenDolPosted almost 4 years ago

  Hey Nafsuki!

  Your solution looks great - really well done! 🤩

  One thing I'd consider paying attention to is the nesting of your SASS. Right now your style on, for example, your h1 only works as long as it is in the .container. Do you really need that depth in your selector or risk the style breaking as soon as it is used outside of the .container?

  In a small project like this obviously it's no problem at all, but keep this in mind on bigger projects! 😄

  1

  ---

  

  Nafsuki•245

  @NafsukiPosted almost 4 years ago

  @GerbenDol Thank you so much for your feedback & suggestion!! You’re right! That exactly what happened to me(breaking styles) especially when I coded media query, and wondering how I could/should improve with nesting.. I removed .container following your advice :) I thought I would always need a container for some reason, but it was unnecessary! I will keep it mind and try not to nest too much! 😄 Thank you very much for a great feedback!

  0
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  Hello, Nafsuki! 👋

  Just wanted to say, nice to see you complete another challenge yet again! 😀 Good work on this one! 👍 Your solution looks great and I like the hover state you gave to the button! 😉

  Keep coding (and happy coding, too)! 😁

  0

  ---

  

  Nafsuki•245

  @NafsukiPosted almost 4 years ago

  @ApplePieGiraffe Hi! Thank you so much for the great feedback again😋 It really motivates me to keep challenging. Happy coding to you too😄!!

  0