@A-amon
Posted
Hello! Looks great~ It's responsive too.
I have a few suggestions for you 😀:
-
Why do you have wrap a line inside h1 with span tag?🤔 You can try using br tag to break the line of the same sentence. (Don't abuse it tho! Check out https://forum.freecodecamp.org/t/should-you-ever-use-the-br-tag/440252/2 for how to use it semantically.)
-
Try to not set styles directly in JS if possible. Add/remove or toggle class instead. E.g. Adding .error class to show error-related styling.
-
Instead of having an unused empty label, set the aria-label attribute on input element. (Doing this allows you to remove the label element)
-
The alt text for your error icon might not be appropriate 🤔, because imagine if the screen reader reads out "Exclamation" or something else to users. Maybe, "Warning", etc. would be better. Or if you wish to hide it, just leave the alt empty. Check this out: https://dev.to/nadiarasul/do-icons-need-alt-attributes-5g52
Marked as helpful
@Sloth247
Posted
@A-amon Hey, I actually wanted to talk about another project. Thank you for pointing out the points;
<br>tags : I think I got some feedback from someone here not to use it to change the row - I know this could be easy if I use it... however, how can you change the font-weight without usingspan?- not styling directly in JS: Okay, understood
- aria-label: I thought all input field should have labels for each?
- alt text: good point, I will fix it from next project.
I saw your solution for Pricing component with toggle by not using JS...! Do you think you can also do this project without using JS? And I tried your solution for the same project but it didn't work on my code😂
@A-amon
Posted
@Sloth247 Nope, it wouldn't be possible to change the font-weight but this project uses same font-weight throughout the heading (If I'm right 😂 I don't trust my sight lmao!) I don't think there's a ban on br tags or something 😂 For me, I think it really depends on how you use it. I think he/she refers to using br tags to replace usage of CSS positioning or margin which is definitely not right.
Yep, every input should have label because it tells the screen reader users what the input is for. But aria-label attribute is also an alternative to it. Only difference would be, what's in aria-label won't be visually available.
Mhmm, I think it could be possible (Don't take my word for this tho! 😂). I have few things in mind 🤔 Maybe soon. You can follow my github in case you wanna see if I manage to do it or not (This sounds so wrong 😂)
Is it in your repo? I can try checking it out~
@Sloth247
Posted
@A-amon Oh ya, I'm sorry I was blind. Please see the comment from matt for my first project about <br> (Please ignore the outcome of the project, it was terrible).
-
aria-label : thanks for pointing out. I will look into the details.
-
Price toggle project: No, I have not saved it in repo but I will let you know when I do via slack message (only if you don't mind)😊
@A-amon
Posted
@Sloth247 No, you're not blind! 😂 Anyway, interesting. But maybe because of abuse of br tags, preferences, etc. (I don't really know anymore 😀)
Ah- Yea, let me know on slack!
