a11y: announce dialog text in package manager #6593
Conversation
beyackle
requested review from
a-b-r-o-w-n,
boydc2014 and
cwhitten
as code owners
| useEffect(() => { | ||
| async function doAnnouncement() { | ||
| announce(props.title); | ||
| } | ||
| doAnnouncement(); | ||
| }, [props.title]); |
There was a problem hiding this comment.
note: this does look strange, but it's the correct way to use an async function inside useEffect.
There was a problem hiding this comment.
This definitely seems incorrect though. What's wrong with this?
useEffect(() => {
announce(props.title);
}, [props.title]);If you wanted to await something, that would be the way to do it, but just invoking an async function shouldn't require doing this.
There was a problem hiding this comment.
I agree it shouldn't, but useEffect has a restriction where it doesn't like something that returns a promise. This is a workaround documented in places like https://www.robinwieruch.de/react-hooks-fetch-data and facebook/react#14326 (where Dan Abramov himself weighs in).
There was a problem hiding this comment.
You can't await a promise within a useEffect block, but you can just call it (like Andy says above)
(use callbacks to handle results if required OR use IIFE to await it like:
(async () => {
await yourAsyncFunction(args);
})();
There was a problem hiding this comment.
According to the type definition, announce isn't async.
There was a problem hiding this comment.
I know it isn't, but trying to do this the obvious way makes the app crash with the same error that blog post mentions: Warning: useEffect function must return a cleanup function or nothing. Promises and useEffect(async () => ...) are not supported, but you can call an async function inside an effect. Pretending it's async and using the IIFE workaround makes this work.
There was a problem hiding this comment.
I think there is a misunderstanding here @beyackle
- is the type definition for
announcewrong and it actually does return a promise? - Even if it does return a promise, it is still ok to invoke an async function inside an effect without using await. (First example I found, but there are others)
@hatpick and I are just saying that in this case, the IIFE is unnecessary because we do not need to await the function.
| useEffect(() => { | ||
| async function doAnnouncement() { | ||
| announce(props.title); | ||
| } | ||
| doAnnouncement(); | ||
| }, [props.title]); |
There was a problem hiding this comment.
This definitely seems incorrect though. What's wrong with this?
useEffect(() => {
announce(props.title);
}, [props.title]);If you wanted to await something, that would be the way to do it, but just invoking an async function shouldn't require doing this.
* Update WorkingModal.tsx * Update WorkingModal.tsx * Update WorkingModal.tsx * Update WorkingModal.tsx * remove unneeded async wrapper Co-authored-by: Andy Brown <[email protected]> Co-authored-by: TJ Durnford <[email protected]>
Description
This adds screen-reader text to the loading-spinner box in the package manager, giving SR-only users feedback that they've successfully gotten a package to be installed or removed.
Task Item
refs #6559