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

Scheduler time zone addons + a11y improvements for form elements. #1949

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

Conversation

amazingphilippe
Copy link
Contributor

@amazingphilippe amazingphilippe commented Sep 16, 2024

Summary | Résumé

This PR adds precisions about time-zone when scheduling messages. It also improves general a11y of the scheduler by

  • Adding a functioning label and hint to the time selector
  • Adding a functioning label and proper markup to the ampm/24h radios
  • Fix a bug with the announcer (was inverted when the page was in FR)
Capture d’écran, le 2024-09-17 à 09 17 12 Capture d’écran, le 2024-09-17 à 09 17 34

Test instructions | Instructions pour tester la modification

  1. When sending in bulk, on the review screen
  2. Open the scheduler
  3. Inspect the Announcer area near the calendar
  4. Toggle the time format, test in FR and EN
  5. Inspect the confirmation message in yellow
  6. Toggle the time format, text in FR and EN
  7. Use VoiceOver, or inspect aria properties of the time selector and the time format radios.

Todo:

  • Validate translations with @mariesohiebezert
  • Maybe hide the label for the time format radios: redundant for sighted users.

Copy link

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -32,7 +32,7 @@ export const Time = ({ name }) => {
});
}}
id={name}
aria-label={name}
aria-describedby={`${name}-hint`}
Copy link
Member

Choose a reason for hiding this comment

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

Where does this id get set? I see a similar looking id over here but that one is static 🤔

Copy link
Member

Choose a reason for hiding this comment

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

When testing with voice over, it correctly read out the hint, so this works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a few lines below and above where you found! I figured they were close enough to just append name with hint

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.

2 participants