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 bugs in new getA11yStatusMessage option in hooks #1585

Conversation

aliceHendicott
Copy link
Contributor

@aliceHendicott aliceHendicott commented Mar 27, 2024

What:

  1. Only update the A11Y message in the document if a non empty message is given
  2. Update the return type to include undefined as an option

Why:

If using the getA11yStatusMessage prop to apply an a11y message for a specific scenario and returning undefined in other scenarios, it is possible for the getA11yStatusMessage function to run once to set the message, then again and sets undefined overriding anything previously set (because of the debounce function used).

I've also updated the types to allow returning of undefined (this is done in your examples of this function).

I've prepared a CodeSandbox showing these two issues.

  • This example uses the useMultipleSelection hook and applies the example getA11yStatusMessage provided in your docs.
  • If you select an option and then remove it by clicking the 'X' on the option, you'll see Voiceover does not announce that it has been removed. This is because an additional state update is overriding this to an empty message.
  • You can also see the type error complaining about returning undefined instead of string

How:

Checklist:

  • Documentation - N/A
  • Tests
  • TypeScript Types
  • Flow Types - N/A
  • Ready to be merged

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5b0d503) to head (1ec26c8).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1585   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1721      1721           
  Branches       515       516    +1     
=========================================
  Hits          1721      1721           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@silviuaavram
Copy link
Collaborator

Hi, @aliceHendicott! Thanks for the effort! I am looking over the v9 changes and you are right, there is an issue over there. It's about the dependency array being passed to the useEffect that triggers the a11y status message.

I will go ahead and fix this asap, already did the changes, and we can check whether this also fixed your issue.

About the undefined part, I would probably just change the docs examples, and return an empty string instead of undefined. This way we don't have to update the types and returning an empty string is not confusing, I believe. What do you think?

We can do a status update once 9.0.1 will land and we can check your use case afterwards. Thank you again!

@silviuaavram
Copy link
Collaborator

#1586

@aliceHendicott
Copy link
Contributor Author

Thanks @silviuaavram! Looks like it's working as expected with v9.0.1 so I'll close off this PR.

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