Interactive-rating-component

Community feedback

• 

  P

  Grog the Frog•480

  @GregLyonsPosted over 2 years ago

  Looks like everything's working properly, and I like how you make the submission button disabled until the user selects a rating; I don't think that was in the challenge itself, so it's a good catch. I have a few suggestions as well:

  1) You should be using semantic HTML (<h1>-<h6> tags, <p>, <button>) instead of <div>'s whenever applicable. This makes your website easier to understand for screen readers and SEO. One example is that your rating buttons should actually be <button> elements instead of <div>'s. Another benefit of this is that users can then use "Tab" and "Shift + Tab" to go through the ratings with a keyboard. Some people physically have trouble using computer mice and need to use a keyboard, whereas others (like myself sometimes) just prefer to use keyboards sometimes for filling out forms (like tabbing between input fields). This behavior is built into <button> and <input> elements and so doesn't require much more work on your end (as long as you remember to do it), but it would be quite hard to code for <div> elements. Using semantic HTML lets you leverage a lot of built-in stuff to make your websites even better without doing more than choosing the correct element.

  2) I believe display: none; applies not only to the element, but to its children as well. This would help you not have to apply your .non-display class to elements one-by-one like you do in your JavaScript. For this to work, you could take your .summary, .thakyou-title, and .thankyou-para elements in wrap them in a <div>, with a class like .thankyou-wrapper. Then, you could wrap the other elements (your initial component with the rating feature) in another <div>, with a class like .rating-wrapper. Then you can just add/remove the .non-display class on those two wrapper <div>'s.

  3) You can read more about display: none; here. Near the middle of the page, you can see that with display: none;, your page will be displayed as if the element isn't there. Thus, these rules in your .non-display class:

  margin: none;
  border: none;
  width: none;
  height: none;
  

  aren't necessary/don't do anything, as your element isn't taking up space when it has display: none;. They would be relevant if you used visibility: hidden; instead, in which case the element disappears but still takes up space, but in that case you may as well just use display: none; in the first place.

  Hope this helps!

  Marked as helpful

  0
• 

  Ivan•2,630

  @isprutfromuaPosted over 2 years ago

  Hi there. You did a good job 😎

  keep improving your programming skills🛠️

  your solution looks great, however, if you want to improve it, you can follow these steps:

  ✅ Fix navigating through page via TAB key

  ✅ Disallow css fonts @import. CSS font @import prevents parallel downloads, use <link> instead.

  ✅ Use relative units for font size, and witdth/height such as ems or rems. While modern browsers can smoothly zoom pixel-based layouts, sizing type in relative units ensures an entire layout can be scaled up or down by simply updating the font-size of the body element.

  I hope my feedback will be helpful. You can mark it as useful if so 👍 it is not difficult for you, but I understand that my efforts have been appreciated

  Good luck and fun coding 🤝⌨️

  Marked as helpful

  0
• 

  Subham Sharma•130

  @subhamenginePosted over 2 years ago

  Thanks for your feedback, it's very very helpful. Next time ill keep all these points in my mind.

  1

  ---

  

  P

  Grog the Frog•480

  @GregLyonsPosted over 2 years ago

  @subhamengine Sure thing, glad I could help!

  0