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/repeat announcements not working #7

Conversation

Luciaisacomputer
Copy link
Contributor

What is this PR for?

This solves the issue where some screen readers, such as VoiceOver, don't repeat an announcement if the text remains the same. It checks to see if the message is the same as what is currently within the components state, and then if it is the same it appends the string with a non-breaking space. I've seen this solution used a couple of times before for the same problem.

Where should the reviewer start?

  1. Open up your local environment and go to the basic test page here: http://127.0.0.1:8080/basic/

What should the reviewer look at?

  1. Start VoiceOver (or any other screen reader tool)
  2. Click on the Trigger New Announcement button
  3. You should hear an announcement
  4. Click on the button again
  5. You should hear the same announcement again.

What gif best describes this PR or how it makes you feel?

Joke gif about the a11y announcer

Additional comments:

I'm still fairly new to React so please let me know if I am breaking any rules here.

Definition of Done:

  • [❓] Is there appropriate test coverage?
  • [❓] Does this pass all automatic linting?
  • [❓] Does the documentation need to be updated?
  • [❌] Does this add new dependencies?

Luke Pettway added 2 commits August 4, 2017 10:59
…od to compare against the state and if it matches, append the string with a nobreak space
@pezfish
Copy link
Contributor

pezfish commented Aug 4, 2017

@LukePettway This is a really great solution. The only comment I would say is to move it into the component itself instead of the demo page. That way whoever is using it doesn't have to rely on a specific implementation.

We probably could check that value on componentWillReceiveProps and update as necessary.

@Luciaisacomputer
Copy link
Contributor Author

Thanks, I'll work on implementing that and let you know when it is ready.

…nd whitespace so that any code using this feature doesn't have to account for identical strings not announcing on various screen readers.
@Luciaisacomputer
Copy link
Contributor Author

@pezfish so it looks like you were right, I just needed to add a state, and then use that in my check to see if the value matched, then use setState() to update the value.

@tikaro
Copy link
Contributor

tikaro commented Aug 8, 2017

FYI, @LukePettway and @pezfish , I just added "branch protection" to the master branch, since that seemed like a pretty good idea (thanks for the suggestion, Luke!)

If I've done this the way I think I have, when pezfish approves the PR, that will unblock merging. We'll see if I've done it right once pezfish approves!

Copy link
Contributor

@pezfish pezfish 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. Thanks.

@Luciaisacomputer Luciaisacomputer merged commit 526a907 into thinkcompany:master Aug 9, 2017
@Luciaisacomputer Luciaisacomputer deleted the fix/repeat-announcements-not-working branch August 9, 2017 18:55
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.

None yet

3 participants