@pikapikamart
Posted
Hey, great work on this one. Desktop layout looks great, just a little bit shorter than the design. Site is responsive and the mobile state looks great as well.
Marta already gave great feedback on this one, just going to add some suggestions as well:
- Since you are using 2 container for the logo and the form itself, I suggest using:
<header /> # contains the logo
<main /> # contains the main component
This markup will let you have landmark wrapping each of the content.
- Remember that a website-logo is one of the meaningful images on a site so use proper
alt
for it. Use the website's name as the value likealt="splitter"
. - When using
img
tag, you don't need to add words that relates to "graphic" such as "logo" and others, sinceimg
is already an image so no need to describe it as one. - It would be great to use a
form
tag to wrap the main component in here since there are data that will be controlled by the user. - Those text like the
bill
andnumber of people
could have been used as thelabel
for each of theinput
that is below them. It would be great to use that so that theinput
will have a labelling text describing the purpose and the info needed for each. - Currently, those error messages are only seen visually but not really linked with their respective
input
tag. A pseudocode of a proper error showing would be like this:
if ( input is wrong )
input.setAttribute("aria-invalid", "true");
input.setAttribute("aria-describedBy", id of the error-message);
else
input.removeAttribute("aria-invalid");
input.removeAttribute("aria-describedBy");
The error-message element should have an id
attribute which is referenced by the aria-describedBy
attribute on the input
element. By doing that, your user will know that the input is wrong because of aria-invalid
and they will know what kind of error they made because of the aria-describedBy
.
- Also, the error-message is still seen by the browser since you are using only a
opacity
styling. When you are hiding an element, adding eithervisibility: hidden
ordisplay: none
on their hidden state is the proper way so that they will be totally hidden for users. - For the tip selection a preferred markup on that one would be to use a
fieldset
tag with alegend
tag as well. Thelegend
text content will be theselect tip
word. You will have to use 5 radio buttons with their respectivelabel
tag:
<fieldset>
<legend>Select Tip</legend>
<div>
<label for="5%">5%</label>
<input type="radio" name="5%" value="5%" id="5%">
</div>
......
</fieldset>
It is preferred because if you think about it, you can only select a single tip percentage right and in a set of radio buttons, you can only select one as well. Using this markup as well will be much clearer for screen-reader users since it will always announce what is selected when user is traversing on the different selections.
- The
custom
input needs to have alabel
on it, though it would be a screen-reader onlylabel
or you can usearia-label
on theinput
as well. The text should describe what is the needed info for theinput
. - You can use heading tag on the
tip amount
andtotal amount
since they give information on what the section would contain, hence the amounts from the calculator. Also, for this, it would be great if you somehow managed to usearia-live
oroutput
for the pricings so that whenever the user is finished doing the calculator, screen-reader will announce the calculated price right away for them. This way, they won't have to navigate and search on what is the result. - Also, the
.attribution
is getting in the way when opening up dev tools at the bottom. I was going to suggest using a flebox withflex-direction: column
on the main container but I saw that your markup looks like this:
<div /> # left side of the calculator
<div /> # right side of the calculator
<div /> # attribution
If you somehow managed to nest the two part of the calculator in a single container, that would be great so that you can remove the position: absolute
on the .attribution
and you can just use the flexbox.
Aside from those, great job again on this one.
Marked as helpful
@soumya495
Posted
@pikamart Thank you for such an in-depth review, there is so much of new stuff that I got to learn from this. I will make sure that I'll apply these when I am building my next project.