CSS grid, SASS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop looks fine, just needed for it to be centered vertically. The responsive state could be better since right now, at 960px going down to 684px, a large section is being hidden thus creating a horizontal scrollbar. The mobile layout looks fine but a small scrollbar is present as well.

  Some other suggestions besides the logic for the tip:

  • The alt for the website-logo should have used alt="splittier", the image already has the text so better using it. Also, as always avoid words that relates to "graphic" such as "logo", always use meaningful words for meaningful images.
  • For the form you are using too much grid, actually you don't really need that in there, since the layout is just one column. Those tips is just one container with a flex.
  • For the bill input add a min=0 attribute so that whenever using arrow-key, the value won't go lower down the specified value which is 0.
  • The dollar icon on the input should have used alt="" and aria-hidden="true" as an extra attribute. Decorative images should be hidden for users.
  • To be honest, I find it hard to use structure html on the tips section, but for me, I would use input type="radio" on the 5 items which is being wrapped by fieldset along with an sr-only legend element. This way, users will know directly what does this set of items will do and will be easier for them to select one. Because using only button does not add extra accessibility features, unless you use aria-live element.
  • The custom input needs to have a label associated with it, the label will be having sr-only class, then the text should describe what does the input does.
  • Again min=0 for the custom input.
  • For the number of people input even if I put 0 or negative values, the error-message doesn't seem to appear, you might want to check that one out.
  • The alt for the icon on the number input should be alt="" and aria-hidden="true".
  • The tip amount and the total could use some heading tags, since it is a section that shows the value.
  • The dollar sign again should be hidden, using the above method that I mentioned.
  • You don't have to use input type="reset" on this one, since the input is not inside the form if you are going to use this, you must include it inside the form to work properly. But since it is not, you might want to just use button element for this one. As an extra accessibility, an aria-live element announcing that the form has reset would be really great.

  Aside from those, great work. But look at the responsiveness that was mentioned above.

  Marked as helpful

  0
• 

  Joako Flores•30

  @hecmen84Posted about 3 years ago

  Hi olabayoji, there is another way to work with buttons that change color when clicked, You can mask them as buttons but they are really radio buttons:

  <ul class="tip-button-container"> <li class="tip-button"> <input type="radio" name="tip" id="t5" value="5"> <label for="t5">5%</label> </li> <li class="tip-button"> <input type="radio" name="tip" id="t10" value="10"> <label for="t10">10%</label> </li> <li class="tip-button"> <input type="radio" name="tip" id="t15" value="15"> <label for="t15">15%</label> </li> <li class="tip-button"> <input type="radio" name="tip" id="t20" value="20"> <label for="t20">20%</label> </li> <li> <input type="text" placeholder="Otro"> </li> </ul>

  the the CSS: .tip-button-container{ display: flex; justify-content: flex-start; align-items: flex-start; flex-wrap: wrap; width: 100%; height: auto; } .tip-button{ display: flex; justify-content: center; align-items: center; } .tip-button-container li { outline:none; margin: 10px 12px 0px 0px; padding: 13px; border:0; width: 30%; height: 40px; background: var(--Very-dark-cyan); position: relative; list-style-type: none; border-radius: 4px; color:#fff; } .tip-button-container label, .tip-button-container input { display: block; position: absolute; top: 0; left: 0; right: 0; bottom: 0; }

  .tip-button-container input[type="radio"] { opacity: 0.01; z-index: 100; }

  .tip-button-container input[type="radio"]:checked+label, .Checked+label { background: var(--Strong-cyan); }

  .tip-button-container label { padding: 5px; border: 0; cursor: pointer; z-index: 90; border-radius: 5px; text-align: center; }

  .tip-button-container label:hover { background:var(--Light-grayish-cyan); color:#333; border: 0; border-radius: 5px; }

  Marked as helpful

  0

  ---

  

  Olabayoji•135

  @OlabayojiPosted about 3 years ago

  @hecmen84 Thank you so much. Using the radio button to select and deselect the tip percentage value is a great idea. Would never have thought of it. 🤝

  0
• Account deleted

  Hi,

  Nice one on completing the challenge, it works but I don't think it's correct;

  • The math seems to be incorrect, like it's not even close;

  1. A bill of $20 and a tip of 25% and 4 people gives me a tip amount / person of $5 and a total / person of $10... as you can see that's way overboard.
  2. The tip is some how correct but you have to divide it by the number of people to get the tip amount / person, so it'll have to be 5 / 4 = $1.25.
  3. For the total / person you first have to divide the bill by the number of people in this case it'll be 20 / 4 = $5, plus the tip amount / person; so the total / person is $5 + $1.25 = $6.25

  And on the other side the styling also needs a little bit of work.

  1

  ---

  

  Olabayoji•135

  @OlabayojiPosted about 3 years ago

  @thulanigamtee haha. I don’t even know what I was thinking 🤦🏾‍♂️. I’d go correct my formula.

  0