Interactive Rating Component using vanilla JavaScript

Community feedback

• 

  T

  Grace•31,530

  @grace-snowPosted about 2 years ago

  Hi

  There are still quite a few issues in this, but it is much improved from before.

  I recommend reading up on modern-css.dev about how to accessibly style radio inputs. What you've got at the moment is not accessible and is far more complicated than it needs to be.

  I also see issues with how you are using explicit heights and widths; and margin/padding.

  This component should not have a height or width. Just a max-width in rem. NO height at all. Let the browser dictate the height needed based off the content inside.

  With padding and margin, remember the box model and keep it simple. All elements inside the component don't touch the edge of the box. So use padding on that box - padding is for internal spacing and does not collapse. For the elements inside the box (heading, paragraph, form...), you want to create space between those elements, so use vertical margins - margin is for external spacing and does collapse.

  The other point which I think has been raised above is how to center your component on the page. Don't use huge margins. Use flex/grid properties along with min-height 100vh on the wrapping element to center it's contents on the screen.

  All the relative/absolute positioning is unnecessary on this. You may use some for the radios but the markup structure would need to change first (see that article I mention above)

  
  /* ratingcomponent.css | https://www.clewisdev.com/FMinteractiveratingcomponent/ratingcomponent.css */
  
  section {
    /* width: 327px; */
    /* height: 360px; */
    /* margin: 0 auto; */
    /* margin-top: 9.625rem; */
    max-width: 20.4rem;
    margin: 1rem; // stop component hitting screen edges
    padding: 2rem;
  }
  
  body {
    min-height: 100vh;
    margin: 0;
    padding: 0;
    display: grid;
    place-content: center;
  }
  
  .star {
    /* position: relative; */
    /* top: 1.5rem; */
    /* left: 1.5rem; */
    /* margin-bottom: 1.875rem; */
    margin-bottom: 1rem;
    display: block;
  }
  
  .title {
    /* padding-left: 1.5rem; */
  }
  
  .subtitle {
    /* padding-top: .4375rem; */
    /* padding-left: 1.5rem; */
    /* padding-right: 1.5rem; */
    margin-top: 1rem;
  }
  
  .input-div {
    /* padding-left: .5rem; */
    margin: 1rem 0 2rem;
  }
  
  [type="radio"] {
    note: this is not how you accessibly hide inputs! opacity 0 is not right;
    note: needs required attribute on one radio in html to prevent submit before completion;
    clip: rect(1px, 1px, 1px, 1px);
    clip-path: inset(50%);
    height: 1px;
    width: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    position: absolute;
  }
  
  [type="radio"] + label {
    note: wrap the input in the label and style the label;
  }
  
  label {
    /* margin-top: .5rem; */
    /* margin-bottom: 2rem; */
  }
  
  [type="radio"] + label::before {
    /* content: ''; */
    /* position: absolute; */
    /* left: -16px; */
    /* top: -14px; */
    /* width: 42px; */
    /* height: 42px; */
  }
  
  .submit-btn {
    /* position: relative; */
    /* left: 2rem; */
    /* width: 85%; */
    /* margin-bottom: 2rem; */
    width: 100%;
  }
  

  Marked as helpful

  1

  ---

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow Thanks again for taking the time for all these extensive notes, I really appreciate it.

  After changing the code: In general, I don't understand how this is going to work with the design parameters that are given in Figma. This is why I usually use explicit sizing because I can't get anything else to work. When I apply what you have here, it works fine for the mobile sizing, but it doesn't work for the desktop sizing. The design parameters and files state that the mobile width is 327px, and the desktop is 412px. When I apply max-width 20.4 rem and min-height: 100vh, the width stays the same whether I am working on mobile or desktop sizing within the browser. However, it does seem to fluctuate better if I remove box-sizing: border-box, but it still isn't those exact sizes. So, what am I doing wrong?

  I'm not understanding the radio button styling quite yet, or the padding and margin, I will take a deeper dive into those to make sure I understand.

  Do you mean moderncss.dev? Do you have any other resources for CSS best practices? I think it can be difficult because it seems like there are so many ways to do the same thing in CSS. The radio button styling that I did came from a YouTube video. It has been a while since I have been through any sort of CSS tutorial or class and it looks like maybe I could use an update. I have never seen some of these selectors before, nor have I seen display grid / place content center. Do you have any experience with or thoughts about Scrimba's program? Thanks again!

  0

  

  T

  Grace•31,530

  @grace-snowPosted about 2 years ago

  @casserole27 Yes, I strongly encourage you to dive more into this. What you have currently is inaccessible and not fully responsive. It needs to be able to adjust and it needs to work (and have visible indicators) for keyboard users.

  You must never limit the height of components containing content, and should be using max-width not width so they can shrink when needed.

  The design is a static representation on a larger phone, but would still need to work down to 320px wide without any horizontal scrolling / overflow. It's up to us as frontend developers to decide how that should happen. I would allow the card to shrink down, make the flex gap between items small and use justify content to then spread them out when there is room.

  Marked as helpful

  1

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow Thanks again, this is so helpful. Thank you for taking the time.

  This gives me a plenty of notes and things to try, will only increase my understanding of CSS, and I'll keep updating the project until it's a best practice. I've been playing with the max-widths more today, it makes more sense, and it seems so much better than working with the explicit dimensions.

  0
• 

  T

  Grace•31,530

  @grace-snowPosted about 2 years ago

  I'm afraid the html is incorrect on this. It must be a form with a fieldset of radio inputs so that the ui works as expected and state is communicated for keyboard and screenreader users etc. It cannot be a load of divs. NEVER ever add interactive behaviour to non-interactive elements. I cannot stress how important this is.

  Marked as helpful

  1

  ---

  

  T

  Grace•31,530

  @grace-snowPosted about 2 years ago

  Also you shouldn't be setting any inline styles in javascript. This is a standard html form and all styling can be done in css using the :checked pseudo state, which is an essential thing to learn how to do.

  The only one tiny bit of js you need on this is to take the chosen value from the formData API and insert it into the thank you screen; Optionally, if you are going to show/hide the form and thank you on the same screen as you are doing here, your js should be toggling a class or hidden attribute on each panel. No inline styles!

  Marked as helpful

  1

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow THANK YOU so much. It makes complete sense when you say the HTML should be a form. It has been a long time since I have coded/practiced a form, so I initially didn't think of it. It also makes sense that interactive behavior should not be added to non-interactive elements.

  0

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow I'm not sure what you mean by setting an inline style in JS. Do you mean things like this? selectionColor1.style.backgroundColor = '#959eac'; I do not know much about pseudo states, but I will take this as an opportunity to learn them so I can make this project in line with best practices.

  0

  

  T

  Grace•31,530

  @grace-snowPosted about 2 years ago

  @casserole27 yes exactly. You almost never need to do .style in Javascript. Keep the concerns separate. Javascript to manipulate the DOM - e.g. toggle classes / attributes - CSS to control all styling

  :checked is just how you target the state of radios or checkboxes.

  Marked as helpful

  1

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow Thanks again for your time, I will continue to work on the project.

  0

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow I have revamped the project. The HTML is a form, I used pseudo-classes and elements to manipulate the styles, and I made the JS a lot simpler.

  My question: my feeling is that I should be submitting via a form action and somehow linking to the thank you state that way rather than what I've done here? However, I can't get the JS to work that way.

  0

  

  T

  Grace•31,530

  @grace-snowPosted about 2 years ago

  @casserole27 I'm still seeing inline styles being set by JS... You should be listening for form submit not a button click and using the formData api

  Marked as helpful

  1

  

  C Lewis•110

  @casserole27Posted about 2 years ago

  @grace-snow Unfortunately, the JS tutorial I took for interactivity was mostly dealing with changing styles and click events. Forms seem so super important though! Your info here and the project itself gives me a much better idea of best practice JS and how to proceed, thanks again!

  0
• 

  Sinka László•470

  @dlxzeus777Posted about 2 years ago

  Hey C Lewis nice looking solution, however you could've just used a for of loop to loop through the buttons and get its value and apply it to the innertext.

  Like this for example:

  for(let btn of buttons) { btn.addEventListener('click', (e) => { const target = e.target.value; console.log(target); selectedNumber.innerText = target; }) }

  1

  ---

  

  Daniel Król•20

  @DanielKrol90Posted about 2 years ago

  @dlxzeus777 I agree, it will make less need to write of code and add "queryselectors". With just short 2 function You can make it :) and add style changer in function, that make it even less need to write of code.

  1
• 

  バレンタイン 😈•64,170

  @VCaramesPosted about 2 years ago

  Hey there! 👋 Here are some suggestions to help improve your code:

  • To properly center your content to your page, you will want to add the following to your <Body> Element (this method uses CSS Grid):

  body {
      min-height: 100vh;
      display: grid;
      place-content: center;
  }
  

  • Change width to max-width in your component’s container to make it responsive. You will also want to delete the height as it is unnecessary.

  • The proper way to build this challenge is to create a Form and inside of it, the “rating buttons” should be built using an Input Radio and it should have a Label Element attached to it.

  If you have any questions or need further clarification, feel free to reach out to me.

  Happy Coding! 🍂🦃

  0