Interactive rating component

Community feedback

• 

  P

  Grog the Frog•480

  @GregLyonsPosted over 2 years ago

  Based on your solution--which works well, by the way--it looks like you have a pretty good grasp of basic DOM manipulations, so good job there. I don't have a single resource that I used to learn DOM manipulations (and part of the reasons I'm doing these challenges is to learn how to manipulate the DOM with vanilla JS instead of using something like React). It looks like this is a good source, but the approach I'm taking right now is to get in practice through these challenges and Google whatever I need as I go. I find that I know enough about what can be done with the DOM so that I know what to Google when I need help.

  I'd say you have a pretty good foundation with all the functions you used here (selecting DOM elements by classes, IDs, etc., adding event listeners), so that in the future if you want to do something you (e.g. listen for a particular event) but don't know the syntax, you could Google, e.g. "javascript listen for <type of event> event" or something like that.

  Another couple notes I have are on your HTML/CSS:

  1) Whenever you use a <section> element, you need an <h1>-<h6> element somewhere within (probably not an <h1> since you really should only have one of those per page); the first such heading element within the <section> will be the name of that section for screen readers and other technologies that are trying to understand your page. For your case, I'd say that "Thank you!" is an appropriate header (<h2> or even <h1> since there's no other <h1> on the page).

  2) Aside from the last point, I'd say you're doing a pretty good job using semantic HTML. I do feel like you're using a bit too many <div>'s though, specifically in your card-state section. It looks like you're using, e.g. .card-container-selected to apply background-color, border-radius, and centering, but that's not really necessary. Since your <p>, .card-state-selected, is already a block element, you can apply the background color and border radius directly to that using CSS.

  Also, in this rule:

  .card-ranking,
  .card-state {
    width          : 100%;
    height         : 100%;
    display        : flex;
    flex-direction : column;
    justify-content: space-between;
  }
  

  if you added an align-items: center;, this would horizontally center the content (horizontally because of flex-direction: column;), and should achieve what you're trying to do with the various place-content: center; and text-align: center; rules. This rule on the flex parent (your main wrapper for each component) would horizontally center its children. Thus, you don't need the wrapper <div>'s around your <p> elements to center them. Writing less CSS/simpler HTML to achieve the same thing in this way will save you a lot of headaches in the long run, especially on larger-scale projects. Of course, sometimes wrapper <div>s are necessary for styling so you should still keep that tool, but in this case they aren't.

  Hope this helps!

  Marked as helpful

  1

  ---

  

  Jose manuel osorio•60

  @jmblack15Posted over 2 years ago

  @GregLyons Wow thank you so much!

  1
• 

  Clément Creusat•1,655

  @ccreusatPosted over 2 years ago

  Hi! Nice job! It's pretty closed to the design :)

  I would suggest few things as improvements:

  • not setting height on your .card but adding a gap: 2rem (~) on your card-ranking
  • width of .card should be max. 412px on desktop
  • submit button has ~30px border-radius; not 15px :)
  • add a bigger line-height on your texts.

  hope this helps. enjoy coding!

  1