Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

a11y: announce dialog text in package manager #6593

Merged
beyackle merged 14 commits into from
Apr 2, 2021
Merged

a11y: announce dialog text in package manager #6593

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
afb4ae6
Update WorkingModal.tsx
beyackle Mar 29, 2021
10150f2
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Mar 30, 2021
610c34b
Update WorkingModal.tsx
beyackle Mar 30, 2021
b96b74f
Update WorkingModal.tsx
beyackle Mar 30, 2021
57add6b
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Mar 30, 2021
43eb81a
Update WorkingModal.tsx
beyackle Mar 30, 2021
f28ca22
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Mar 31, 2021
ce9c001
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Mar 31, 2021
b59370f
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Mar 31, 2021
2a963f0
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Apr 1, 2021
54b91fe
Merge branch 'main' into beyackle/announceInsidePackageManager
beyackle Apr 1, 2021
ece59ac
remove unneeded async wrapper
beyackle Apr 1, 2021
4dd97b4
Merge branch 'main' into beyackle/announceInsidePackageManager
a-b-r-o-w-n Apr 2, 2021
e6d672d
Merge branch 'main' into beyackle/announceInsidePackageManager
tdurnford Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions extensions/packageManager/src/components/WorkingModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,25 @@
import { jsx } from '@emotion/core';
import { Dialog, DialogType } from 'office-ui-fabric-react';
import { LoadingSpinner } from '@bfc/ui-shared';
import { useApplicationApi } from '@bfc/extension-client';

import { modalControlGroup } from './styles';
import { useEffect } from 'react';
Comment thread
beyackle marked this conversation as resolved.
Outdated

export interface WorkingModalProps {
hidden: boolean;
title: string;
}
export const WorkingModal: React.FC<WorkingModalProps> = (props) => {
const { announce } = useApplicationApi();

useEffect(() => {
async function doAnnouncement() {
announce(props.title);
}
doAnnouncement();
}, [props.title]);
Comment on lines +20 to +22

@beyackle beyackle Mar 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

note: this does look strange, but it's the correct way to use an async function inside useEffect.

@a-b-r-o-w-n a-b-r-o-w-n Mar 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@beyackle beyackle Mar 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 react/react#14326 (where Dan Abramov himself weighs in).

@hatpick hatpick Mar 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
})();

@tdurnford tdurnford Apr 1, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

According to the type definition, announce isn't async.

BotFramework-Composer/Composer/packages/types/src/shell.ts

Line 70 in 90a3320

announce: (message: string) => void;

@beyackle beyackle Apr 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@a-b-r-o-w-n a-b-r-o-w-n Apr 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think there is a misunderstanding here @beyackle

  1. is the type definition for announce wrong and it actually does return a promise?
  2. 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.


return (
<Dialog
dialogContentProps={{
Expand Down