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 sign-up modal #7966

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

Tlazypanda
Copy link
Collaborator

Fixes #7964
Improved accessibility of Sign up modal by adding missing form links, fixing orphaned form links, adding aria-labels for social icons and add alternative text for profile photo.

  • 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

Screenshots

Before

7964_before

After

7964_after

Thanks!

@Tlazypanda Tlazypanda requested review from a team as code owners May 28, 2020 16:08
@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7966      +/-   ##
==========================================
+ 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 ? ✌️

@VladimirMikulic
Copy link
Contributor

@Tlazypanda great work! Would you be able to resolve these 7 errors and 19 warnings so its once and for all? Thanks.

@Tlazypanda
Copy link
Collaborator Author

@VladimirMikulic Hey! The rest of these errors are flagged from the rest of the page for which I will be opening different issues(for ex - header one) and of the warnings apart from the ones in this page the rest are alright since they are -

  • Tab index (which works alright)
  • Missing fieldset (cant be applied here since we require a legend text so applying it can mess up the styling)
  • Underlined text (again part of the styling we can lose this one but want to know if everyone is okay with that)

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.

Looks good for a first iteration :)

@jywarren
Copy link
Member

Perhaps we can ask some fellow Outreachy folks to just take a quick look over this so you all are connected a bit? I'll request a review from @Shreyaa-s and @Shulammite-Aso -- we don't have to for every PR but it's nice to look out for one anothers' work every so often! And, feel free to help review some of their recent work too -- do you all have one that @Tlazypanda might be able to help look over?

Thanks, all!!! 🎉 ❤️

@@ -21,7 +21,7 @@
<div class="row">
<div class="col-md-6">
<div class="form-group" id="username_div">
<label for="username"><%= translation('user_sessions.new.username') %></label>
<label for="username-signup"><%= translation('user_sessions.new.username') %></label>
Copy link
Member

Choose a reason for hiding this comment

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

Just checking if this has affected our client-side validation at all? I would guess not but good to note this so people feel secure about it. Thank you!!!

@@ -48,7 +49,7 @@

<div class="form-group row">
<div class="form-group col-md-6">
<label for="password"><%= translation('users._form.create_password') %></label>
<label for="password1"><%= translation('users._form.create_password') %></label>
Copy link
Member

Choose a reason for hiding this comment

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

Same question about client-side validation!

@@ -59,7 +60,7 @@
</div>

<div class="form-group col-md-6">
<label for="password_confirmation"><%= translation('users._form.password_confirmation') %></label>
<label for="password-confirmation"><%= translation('users._form.password_confirmation') %></label>
Copy link
Member

Choose a reason for hiding this comment

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

and here!

@@ -72,10 +73,11 @@

<div class="form-group row">
<div class="col-md-12">
<label for="user_bio"><%= translation('users._form.bio') %></label>
<label for="user-bio"><%= translation('users._form.bio') %></label>
Copy link
Member

Choose a reason for hiding this comment

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

sorry, one more!

@@ -16,6 +14,7 @@
<div class="btn-group d-flex" role="group">
<% [0,1].each_with_index do |s, j| %><% statement = turingtest[i][s] %>
<button type="button" class="col-5 btn btn-outline-secondary" style="font-size:3em;text-align:left;<% if i.odd? %> background:#eef;<% end %>" id="spamaway-<%= caller %>-<%= i.to_s + j.to_s %>">
<label id="selectAnimal" for="spamaway_statement_<%= caller %>_<%= i.to_s + j.to_s %>">
Copy link
Member

Choose a reason for hiding this comment

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

🐯 🐘 🐻 😻

@shreyaa-s-zz
Copy link
Collaborator

I think changing the labels shouldn't interfere with the validation since we use getElementById or name there. Looks good to me. 👍

Copy link
Collaborator

@Shulammite-Aso Shulammite-Aso left a comment

Choose a reason for hiding this comment

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

This looks clean.🎉🎉

@Tlazypanda
Copy link
Collaborator Author

Hey @Shreyaa-s @Shulammite-Aso @jywarren! Thanks for your reviews ❤️ And I tested for the client side validation with a new user works fine 😅

@jywarren jywarren merged commit 99241cb into publiclab:master Jun 3, 2020
@jywarren
Copy link
Member

jywarren commented Jun 3, 2020

Awesome! Please give this one last test at stable.publiclab.org -- thank you!

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 Sign Up Modal
5 participants