Design comparison
Solution retrospective
Any feedback would be great!
Community feedback
- @grace-snowPosted over 2 years ago
Hello
This looks really great!
I looked at code for one component and see some accessibility issues though
- Aria label value is not code for a computer , it's human readable text. “menu-toggle-btn” would not be read correctly and would make no sense
- The links in the nav should be links not buttons. That's really important as they have very different meanings
- a nav toggle button should actually be inside the nav, and you should be toggling the ul. This is because nav is a landmark, which screenreaders use to jump to common content sections. They would not be able to find the nav in this on first load
- In a disclosure pattern it's essential to use aria-expanded on the button, toggling between true and false to communicate state (and preferably aria-controls as well, pointing to the ID of the content being toggled)
It’s also really important that you make sure content changes on the screen are announced. That’s not so necessary for the nav toggle, but really important with the shortened link. Add an aria-live attribute to the div wrapping the results (the shortened link). That div would need to be on the page at all times (not a conditional or display none), just the content within it would change. This aria live attribute is a way of saying “announce this when the content changes”
I hope these are helpful learning points. Good luck
Marked as helpful2@arshGoyalDevPosted over 2 years ago@grace-snow Thank you very much for the feedback. I will fix all these issues as soon as possible.
0
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