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

fix(curriculum): replace D.O.B. acronym in accessibility quiz #54829

Merged
merged 8 commits into from
Jul 31, 2024

Conversation

Supravisor
Copy link
Contributor

@Supravisor Supravisor commented May 16, 2024

Checklist:

Related to #47830

@github-actions github-actions bot added the scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. label May 16, 2024
@Supravisor Supravisor marked this pull request as ready for review May 16, 2024 06:51
@huyenltnguyen huyenltnguyen added status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. labels May 16, 2024
@Supravisor
Copy link
Contributor Author

Is it okay for this PR to proceed without further changes?

Then the should fix can be applied on Question 1 and Question 2 using the sr-only class, in a separate PR.
Otherwise another issue may need to merge into this one.

@bbsmooth
Copy link
Contributor

bbsmooth commented Jun 3, 2024

Is it okay for this PR to proceed without further changes?

I would vote no.

"Although the Date of Birth text is descriptive, it can be problematic for visually impaired users."

This doesn't make sense. Date of Birth is not problematic for visually impaired users. Also, you wouldn't add visually hidden text that is exactly the same as the visible text. That's just unnecessary duplication for screen reader users.

It's not just a matter of changing D.O.B to Date of Birth. This entire step needs to be reworked.

@Supravisor
Copy link
Contributor Author

Is it okay for this PR to proceed without further changes?

Then the should fix can be applied on Question 1 and Question 2 using the sr-only class, in a separate PR. Otherwise another issue may need to merge into this one.

Is it okay for this PR to proceed without further changes?

I would vote no.

"Although the Date of Birth text is descriptive, it can be problematic for visually impaired users."

This doesn't make sense. Date of Birth is not problematic for visually impaired users. Also, you wouldn't add visually hidden text that is exactly the same as the visible text. That's just unnecessary duplication for screen reader users.

It's not just a matter of changing D.O.B to Date of Birth. This entire step needs to be reworked.

My question was "Is it okay for this PR to proceed without further changes?" - MUST FIX No 4. This way another PR can be made to add sr-only class for Question 1 and Question 2 - Should fix No 1.

naomi-lgbt requested that separate PRs are created to address ten identified issues.

Although several steps may require multiple rework, a way forward without merging one or more PRs looks tenuous.

Note: this PR already has one approval.

If this PR receives a second review approval, a pathway forward is outlined as follows:

  1. MUST FIX No.3: The Question 1 and Question 2 text look like headings and should probably be marked up as headings.
  2. Should Fix No. 1: "Question 1" and "Question 2" sr-only text be added to the beginning of their respective questions so that screen reader users navigating by tab stop will hear the question number as well as the question text.

@bbsmooth
Copy link
Contributor

My question was "Is it okay for this PR to proceed without further changes?"

And my answer is that it doesn't make sense to merge this PR as it is without further changes because it is wrong.

"Although the Date of Birth text is descriptive, it can be problematic for visually impaired users. One way to get around such an issue is to add text only a screen reader can read."

This is just wrong. "Date of Birth" is not problematic for blind people. You would not add text that only a screen reader can read. If you ask me, the changes in this PR make this step worse and I'd rather leave the step as it is now.

@huyenltnguyen huyenltnguyen added status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP and removed status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. labels Jun 19, 2024
Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

naomi-lgbt requested that separate PRs are created to address ten identified issues.

Yes, but if the changes don't make sense to separate, then this doesn't apply.

In this case, it seems like we need to resolve multiple things at once.

  • Instead of using D.O.B. for the label, use Date of Birth
  • Change the screen reader text steps to apply Question 1 and Question 2 to the labels

@ojeytonwilliams
Copy link
Contributor

Hey @Supravisor any thought on Naomi's comments?

@Supravisor
Copy link
Contributor Author

I'll start on the suggestions in the next few days.

Supravisor and others added 3 commits July 31, 2024 12:49
…n-accessibility-by-building-a-quiz/6143956ed76ed60e012faa51.md

Co-authored-by: Naomi the Technomancer <[email protected]>
…n-accessibility-by-building-a-quiz/6143956ed76ed60e012faa51.md

Co-authored-by: Naomi the Technomancer <[email protected]>
…n-accessibility-by-building-a-quiz/614396f7ae83f20ea6f9f4b3.md

Co-authored-by: Naomi the Technomancer <[email protected]>
@naomi-lgbt naomi-lgbt merged commit 48e8653 into freeCodeCamp:main Jul 31, 2024
22 checks passed
@Supravisor Supravisor deleted the quiz-DateOfBirth branch July 31, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants