Skip to content
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

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

Tlazypanda
Copy link
Collaborator

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.

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

Screenshot

Before

7960_before
After

7960_after

Thanks!

@Tlazypanda Tlazypanda requested a review from a team as a code owner May 27, 2020 20:48
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #7962 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
app/controllers/stats_controller.rb 73.62% <100.00%> (+0.29%) ⬆️
app/api/srch/search.rb 70.06% <0.00%> (+3.82%) ⬆️
app/services/execute_search.rb 94.44% <0.00%> (+5.55%) ⬆️

@Tlazypanda
Copy link
Collaborator Author

@VladimirMikulic can you kindly review? Thanks ✌️

@@ -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">
Copy link
Contributor

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?

Copy link
Collaborator Author

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 🎉

Copy link
Contributor

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 :)

Copy link
Collaborator Author

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

Copy link
Contributor

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.

Copy link
Contributor

@VladimirMikulic VladimirMikulic left a 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.

@Tlazypanda
Copy link
Collaborator Author

@VladimirMikulic Have left some comments 😄 Thanks ✌️

Copy link
Collaborator

@cesswairimu cesswairimu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks both 🚀

@cesswairimu
Copy link
Collaborator

@Tlazypanda could we please get the screenshot for this..

@Tlazypanda
Copy link
Collaborator Author

@Tlazypanda could we please get the screenshot for this..

@cesswairimu Here you go! 😄

Before

7960_before_1

After

7960_after_1

@cesswairimu
Copy link
Collaborator

Great thanks 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Accessibility of Navbar
3 participants