@pikapikamart
Posted
Hey, awesome work on this one. Layout in desktop is small, did you code this while you are zoomed in though? Making it larger would be really great. Currently it is not responsive enough since if you go at around 640px going down, content are being hidden. Mobile layout though looks good.
Some suggestions would be:
- You don't need the
h1in theheadersince there is the website-logo that will give the name. For theh1, you should have used that inside themainas the first text-content element. Using like a text-content of "frontendmentor tip calculator example". - Website-logo
imgshould be using the website's name as thealtlikealt="splitter". Remember that a website's logo is meaningful so always make sure it uses the properaltvalue. - Those
billandnumber of peopleshould be alabelfor theinputbelow them and not a heading tag. - The dollar-icon as well the person-icon on each input should have
aria-hidden="true"to be hidden totally for screen-reader users. - For the all the
inputused in here, you should have usedtype="number"so that you won't have to error-checking for a value that is not a number. Use aminand attribute as well so that there will be a minimum value to be chosen upon. - Also if you are making the
inputlook like it has an error, add aaria-invalid="true"on it so that it will be clear and you could use that as a styling hook likeinput[aria-invalid="true"]{} - For the
buttonsection, maybe making each:focus-visiblestate more visible since right now it is hard to tell whatbuttonam I in. - Since you used
buttonin here, you could nest them inside aulsince those are "list" ofbuttonand it will give extra information to the user on how manybuttonare there. Thebuttonyou would need to have anaria-liveelement that will announce the certainbuttonhas been pressed or selected, becausebuttonalone is not informative. I haven't tackled this challenge yet so I can't provide a sample hehe. - The custom-input needs to have an associated
labeltag on it. Since there are visible-label, thelabelwould be a screen-reader only label, meaning it would make user of likesr-onlyclass. The text-content should describe what theinputneeds like the value on theplaceholderor likeadd custom value. - Number of people
inputhas nolabel, use the same method above as well. - Again when wrapping a text-content do not just use
spanto wrap it, use meaningful element like aptag if it just a regular text or heading tag if it is an heading. - Another idea as well is to make use of
aria-liveas well dictating that the calculator has reset when the reset-button is toggled, just to make it more accessible. - Lastly, addressing the responsiveness since content are being hidden on that screen-width that I said above.
Aside from those, again great job on this one.
@mikeyxx
Posted
@pikamart As usual, thank you so much. I like how much you go into the details. I actually just felt the desktop layout was big enough for a calculator but looking at it again, I see your point. I will make all the edits you suggested.
Thank you:)
@mikeyxx
Posted
@pikamart Is it compulsory to use both alt="" and aria-hidden="true" for an image? Can't I just use only alt=""? and in place of <span>, can I use <small> instead of <p>?
@pikapikamart
Posted
@mikeyxx RIght now yes, because if you only use alt="" for a decorative image, VoiceOver in apple will still read the image, that is why adding aria-hidden="true" will completely hide the element for screen-reader.
When wrapping a text, always use p tag or other meaningful element. You can use span or small but inside the meaningful element.
<p>I am a p tag</p>
<p> I have a
<small> small </small>
tag
<p>
@mikeyxx
Posted
@pikamart I've made all the changes you suggested. Debugging is hell!! but I learnt a ton. Thank you. Next I will be working on the time tracking dashboard challenge hub.
