Design comparison
Community feedback
- @elaineleungPosted over 2 years ago
Hi Joshua, excellent work here on this rating component 🙂
I also built mine with accessibility in view, and I'm always curious to see how other developers wrote their code. I just tested out your component and found that the only thing I could use the tab key on were the submit button and links. I think it's because your radio buttons have a
display: none
, which makes them hidden from the browser, and so while they can be clickable, they won't be tabable. If you want to make them tabable also, you can try this, which is what I used in this challenge to hide the clickable radio :[type=radio] { -webkit-appearance: none; -moz-appearance: none; appearance: none; display: inline-block; background-color: inherit; border: 0; border: 0; margin: 0; outline: 0; }
I like your idea of using a disabled button, which is something I actually used on my CodePen example for this component but not in my actual solution, so thanks for the reminder that I need to change that 😅. A suggestion I have is to add a
cursor: pointer
on the labels and buttons, as it's always good to have the pointer for interactive elements.Lastly, on the point about
fieldset
: The tag is actually more for grouping elements and controls for forms (hence the name "field set"), and the radios can still be selected one at a time even without it. What allows them to function as a group is really the "name" attribute. I used arole="radiogroup"
on my div instead of afieldset
tag, but even if I remove the radiogroup role, the buttons can still work and can be tabbed; it's only when I remove thename
from the input that the buttons won't work as a group anymore.Great job, and I'm really happy to see other developers trying to make components more accessible, so keep going 😊
Marked as helpful1@hewhodevsPosted over 2 years ago@elaineleung AH! I completely forgot about adding keyboard navigation / tab ordering + the cursor: pointer on hover. Thanks so much for raising those points, which reminds me I need to write down a pre-launch checklist :). I'll be sure to fix it up and review the radio styles too.
Thanks for the clarification about the fieldset tag too. I'll update my solution description to clarify that point better.
Thanks Elaine, for taking the time to check out my solution and provide such valuable feedback!
1@hewhodevsPosted over 2 years ago@elaineleung Thanks again for the feedback. I've fixed these areas in my solution now as follows:
-
Now hiding the radios accessibly using a .visually-hidden class based on your recommended styles and the example from https://design-system.w3.org/styles/how-to-hide-and-show-things.html. Keyboard navigation now successfully works!
-
Added focus styles for the label to make focus state clear to the user on the radio buttons (I admit, I had to look at your stylesheet for some help with that, I was using the wrong combinator in my selector, but now it works!).
-
Cursor pointer styles now included for the radio's and submit on hover. The submit button will however show the "not-allowed" cursor initially whilst it is disabled, to make it clear that a rating needs to be selected first.
-
Replaced the fieldset tag with a div instead to group the radio's. After reading the MDN docs again, I realised it's not intended to use a fieldset tag without it's nested legend tag (as I had done) and since no legend is needed in this project, a div was the better choice.
I still want to add some smooth transitions / on click effects at some point (I like your solutions button on click background fade effect, it's really nice & smooth!) but for now this will do.
1@elaineleungPosted over 2 years ago@hewhodevs I just had a look and it's working great! Good to know that you had used the visually-hidden class, which is what I normally use, just never thought it could be used for radio buttons! Will try that next time, and thanks for sharing the link as well.
Yes, the combinator selectors can be tricky! It gets worse when there are pseudo classes thrown in. Glad my CSS gave you some ideas! You can also try changing the focus style on the button, which takes the default style right now. But other than that I think everything's looking awesome 😊
Marked as helpful1 -
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord