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

Sc 412 stepper accessibility #5485

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

jachan
Copy link
Contributor

@jachan jachan commented Jan 30, 2025

Link to pivotal/JIRA issue

This issue is public! Highly recommend that both CFA and NJ reviewers check out the ticket.

Is PM acceptance required? (delete one)

  • No - merge after code review approval

Reminder: merge main into this branch and get green tests before merging to main

What was done?

  • Removed title attribute, which has inconsistent accessibility support (does not work for NVDA on Windows)
  • Added text description of progress to

    tag so screen readers can convey the same information to users

  • Added Spanish translation for "Step X of Y", approved by OOI Spanish Content team ✅

How to test?

  • Validated that stepper component attribute does not have title element

  • Validated that step description now includes total step count

  • Use any persona, navigate to after DF import to display stepper component

  • Step through tax flow and verify that stepper component behaves correctly

  • Download NVDA on Windows state laptop, check heroku deploy and validate that screen reader issue no longer exists.

Screenshots (for visual changes)

  • Before (turn sound on)
SC.412.-.before.video.win.mp4
SC.412.-.before.video.mac.mov
  • After
    Video to come
image image

Copy link

Heroku app: https://gyr-review-app-5485-a91408490be0.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5485 (optionally add --tail)

@@ -4,40 +4,40 @@
describe "Flow" do
it "Flow has not changed" do
expect(Navigation::StateFileAzQuestionNavigation::FLOW).to eq([
StateFile::Questions::EligibleController,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto-linter changed indentation to match line 6. Happy to change it back before merging if this bothers folks.

Copy link
Contributor

Choose a reason for hiding this comment

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

oof i am sorry to report it does bother me 🙈; would you mind just assigning the array to a variable and putting the variable in the expectation? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries! (It was bugging me too haha)

section_4: Transferir tus datos
section_5: Completa tu declaración de impuestos estatales
section_6: Envía tus impuestos estatales
step_description: 'Paso %{current_step} de %{total_steps}: '
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] Our conversation about translation + Damian's video make me think that we can maybe keep using the word "section"/"sección" here and in the en version? Then it's less of a change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! This is going through CFA approval with Anu right now, I will bring up this option as an alternative!

@@ -58,7 +58,7 @@
it "returns the correct progress" do
progress = Navigation::StateFileAzQuestionNavigation.get_progress(StateFile::Questions::FederalInfoController)
expect(progress).to eq({
title: "Section 5: Complete your state tax return",
title: "Complete your state tax return",
Copy link
Contributor

Choose a reason for hiding this comment

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

[microscopic dust] I wonder if it's worth changing "title" to something like "caption" to avoid confusion with the title attribute or page title. Not adamant about this opinion though

Copy link
Contributor

@jenny-heath jenny-heath 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 overall code-wise, will hold off on approving until we have design/product review of the new copy!

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.

3 participants