-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve accessibility of navbar #7962
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7962 +/- ##
==========================================
+ Coverage 82.04% 82.17% +0.12%
==========================================
Files 97 97
Lines 5642 5643 +1
==========================================
+ Hits 4629 4637 +8
+ Misses 1013 1006 -7
|
@VladimirMikulic can you kindly review? Thanks ✌️ |
app/views/layouts/_header.html.erb
Outdated
@@ -17,9 +17,10 @@ | |||
<div class="collapse navbar-collapse" id="header-navbar-collapse"> | |||
<form class="form-inline mr-3" id="searchform" autocomplete="off"> | |||
<div class="input-group"> | |||
<label id="searchLabel" for="searchform_input"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label tag is empty and not closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VladimirMikulic The reason I added this without any text like this is because I didn't want to change the page content while at the same time add a form-label. And the reason I added this is because some old screen readers don't recognize the aria assistive apis 😅 Thanks a lot for the review 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not closing tag is a bad practice. It can cause structural and styling issues down the road.
In my opinion, aria-label
is the way to go. Messing with the DOM can be dangerous.
We can't forever be backwards compatible :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VladimirMikulic That's right 😅 I will update this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. Take your time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please resolve the issues.
@VladimirMikulic Have left some comments 😄 Thanks ✌️ |
990691e
to
37794a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thanks both 🚀
@Tlazypanda could we please get the screenshot for this.. |
@cesswairimu Here you go! 😄 Before After |
Great thanks 🎉 |
Fixes #7960
Improved accessibility of navbar by fixing empty search button problem, adding alt-text to profile image and adding form label to search form.
rake test
@publiclab/reviewers
for help, in a comment belowScreenshot
Before
After
Thanks!