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

chore/5057-streamlineUseIsScreenReaderEnabledHook #10362

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

alexandec
Copy link
Contributor

@alexandec alexandec commented Dec 10, 2024

Description of Change

Switch to the useIsScreenReaderEnabled hook in the component library. Remove our version of the hook.

Screenshots/Video

Toggling VoiceOver:

voiceover-toggle.mov

Console output when toggling, showing events:

setAnalyticsUserProperty vama_uses_screen_reader true
setAnalyticsUserProperty vama_uses_screen_reader false
setAnalyticsUserProperty vama_uses_screen_reader true

Testing

  • Tested on iOS
  • Tested on Android

Reviewer Validations

Check usage of useIsScreenReaderEnabled for regressions:

  • CategoryLandingTemplate
  • FeatureLandingAndChildTemplate
  • HeaderBanner
  • CollapsibleMessage
  • useAutoScrollToElement (AlertWithHaptics, ClaimPhase)
  • useAccessibilityFocus (BackButton, SnackBar, HeaderBanner, GenericOnboarding)

vama_uses_screen_reader event is also good to check, but I don't think QA has a good way to check it. In the console I can see that it's being sent every time the screen reader is turned off/on. I can also see that it's sent on app load.

PR Checklist

Reviewer: Confirm the items below as you review

  • PR is connected to issue(s)
  • Tests are included to cover this change (when possible)
  • No magic strings (All string unions follow the Union -> Constant type pattern)
  • No secrets or API keys are checked in
  • All imports are absolute (no relative imports)
  • New functions and Redux work have proper TSDoc annotations

For QA

Run a build for this branch

@alexandec
Copy link
Contributor Author

@TimRoe since you worked on this in the past, if you could take a look, that'd be great

@TimRoe
Copy link
Contributor

TimRoe commented Dec 10, 2024

@alexandec I can, but seeing this will also mention that we ended up adding this hook to the design system and it is being exported as useIsScreenReaderEnabled from the components package so it could potentially be simplified further to relying on that and removing the hook from the app entirely.

@alexandec
Copy link
Contributor Author

@TimRoe thanks for the info, I've updated the PR to use the hook from the component library instead. I removed the hook and the hook docs from our repo

Copy link
Contributor

@TimRoe TimRoe left a comment

Choose a reason for hiding this comment

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

Code-wise, looks good to me.

Only critique is the PR write-up not including that testing should also validate the useAccessibilityFocus use cases as another hook leveraging the existing one besides the useAutoScrollToElement mentioned.

@github-actions github-actions bot added the FE-With QA A PR waiting for QA Activity label Dec 10, 2024
@github-actions github-actions bot added FE-Ready to Merge and removed FE-With QA A PR waiting for QA Activity labels Dec 11, 2024
@alexandec alexandec merged commit 680bfb4 into develop Dec 11, 2024
67 of 95 checks passed
@alexandec alexandec deleted the chore/5057-streamlineUseIsScreenReaderEnabledHook branch December 11, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants