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

Add support for back button accessibility trait. #219

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RoyalPineapple
Copy link
Collaborator

@RoyalPineapple RoyalPineapple commented Jul 30, 2024

Navigation controller back buttons contain a secret menu trait which calls them out with a distinct utterance of "back button" rather than simply "button". This change adds support for detecting this trait and provides the matching utterance string.

@RoyalPineapple RoyalPineapple requested review from elzinke and meherkasam and removed request for elzinke July 30, 2024 17:36
Copy link
Collaborator

@kyleve kyleve left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@@ -10,6 +10,9 @@
/* Description for the 'button' accessibility trait */
"trait.button.description" = "Taste.";

/* Description for the 'back button' accessibility trait */
"trait.backbutton.description" = "Zurück Taste.";
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

@RoyalPineapple RoyalPineapple changed the title Alex/back button Add support for back button accessibility trait. Aug 1, 2024
Copy link
Collaborator

@NickEntin NickEntin left a comment

Choose a reason for hiding this comment

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

Logic looks good! Can we add a snapshot test or two (maybe of a nav controller with a second vc pushed, with and without a title on the previous item) to test this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay, thanks for doing this!

@RoyalPineapple
Copy link
Collaborator Author

Logic looks good! Can we add a snapshot test or two (maybe of a nav controller with a second vc pushed, with and without a title on the previous item) to test this?

@NickEntin I added snapshot tests as you suggested. Do you mind a re-review?

Copy link
Collaborator

@kyleve kyleve left a comment

Choose a reason for hiding this comment

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

LGTM!

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