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.

Sign up for GitHub

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

feat(notification-system): add screen reader announcer #745

Merged
merged 4 commits into from
Jan 2, 2025
Merged

feat(notification-system): add screen reader announcer #745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a60607
feat(notification-system): add screen reader announcer
gabrielchl Dec 27, 2024
094d701
fix: tests
gabrielchl Jan 2, 2025
20be6aa
fix: stories
gabrielchl Jan 2, 2025
d9fd0af
fix: address comment
gabrielchl Jan 2, 2025
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
24 changes: 15 additions & 9 deletions src/components/NotificationSystem/NotificationSystem.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ const Example = Template<NotificationSystemProps>((args: NotificationSystemProps
content="I'm a low attention notification"
toastCloseButtonLabel="Close notification"
aria-label="Some notification"
/>
/>,
{ notificationSystemId: 'id' }
)
}
>
Trigger a new low attention notification
</ButtonPill>
<NotificationSystem {...args} limit={3} />
<NotificationSystem {...args} limit={3} id="id" />
</>
);
}).bind({});
Expand All @@ -64,13 +65,13 @@ const Important = Template<NotificationSystemProps>((args: NotificationSystemPro
closeButtonText="Close"
toastCloseButtonLabel="Close notification"
/>,
{ attention: ATTENTION.MEDIUM }
{ attention: ATTENTION.MEDIUM, notificationSystemId: 'id' }
)
}
>
Trigger a new medium attention notification
</ButtonPill>
<NotificationSystem {...args} />
<NotificationSystem {...args} id="id" />
</>
);
}).bind({});
Expand All @@ -92,7 +93,8 @@ const Mixed = Template<NotificationSystemProps>((args: NotificationSystemProps)
content="I'm a low attention notification"
toastCloseButtonLabel="Close notification"
aria-label="Some notification"
/>
/>,
{ notificationSystemId: 'id' }
)
}
>
Expand All @@ -109,13 +111,14 @@ const Mixed = Template<NotificationSystemProps>((args: NotificationSystemProps)
/>,
{
attention: ATTENTION.MEDIUM,
notificationSystemId: 'id',
}
)
}
>
Trigger a new medium attention notification
</ButtonPill>
<NotificationSystem {...args} />
<NotificationSystem {...args} id="id" />
</>
);
}).bind({});
Expand Down Expand Up @@ -160,12 +163,13 @@ const UpdateContent = Template<NotificationSystemProps>((args: NotificationSyste
// if toast active and numberRaisedHand higher than 0, update the existing toast
NotificationSystem.update(toastId.current, {
render: Notification,
notificationSystemId: 'id',
});
}
} else {
if (numberRaisedHand > 0) {
// if no toast is there and number of raised hand is higher than 0, show new notification
toastId.current = NotificationSystem.notify(Notification);
toastId.current = NotificationSystem.notify(Notification, { notificationSystemId: 'id' });
}
}
}, [Notification, numberRaisedHand]);
Expand All @@ -174,7 +178,7 @@ const UpdateContent = Template<NotificationSystemProps>((args: NotificationSyste
<>
<ButtonPill onPress={increaseCount}>Increase number raised hand</ButtonPill>
<ButtonPill onPress={decreaseCount}>Decrease number raised hand</ButtonPill>
<NotificationSystem {...args} />
<NotificationSystem {...args} id="id" />
</>
);
}).bind({});
Expand Down Expand Up @@ -216,13 +220,15 @@ const ResetTimer = Template<NotificationSystemProps>((args: NotificationSystemPr
toastId.current = NotificationSystem.notify(Notification, {
autoClose: 5000,
onClose: handleClose,
notificationSystemId: 'id',
});
setNotificationIsShown(true);
}, [Notification, handleClose]);

const resetTimer = React.useCallback(() => {
NotificationSystem.update(toastId.current, {
autoClose: 5000,
notificationSystemId: 'id',
});
}, []);

Expand All @@ -234,7 +240,7 @@ const ResetTimer = Template<NotificationSystemProps>((args: NotificationSystemPr
<ButtonPill onPress={resetTimer}>Reset Timer</ButtonPill>
<Text tagName="p">Verify the reset with the help of this clock:</Text>
<Text tagName="p">{currentTime.toLocaleTimeString()}</Text>
<NotificationSystem {...args} />
<NotificationSystem {...args} id="id" />
</>
);
}).bind({});
Expand Down
42 changes: 23 additions & 19 deletions src/components/NotificationSystem/NotificationSystem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { CompoundProps, Props } from './NotificationSystem.types';

import 'react-toastify/dist/ReactToastify.css';
import './NotificationSystem.style.scss';
import ScreenReaderAnnouncer from '../ScreenReaderAnnouncer';

/**
* The `<NotificationSystem />` component allows consumers to trigger notifications on the defined position on the screen.
Expand Down Expand Up @@ -55,25 +56,28 @@ const NotificationSystem: FC<Props> & CompoundProps = (props: Props) => {
: [ATTENTION.LOW, ATTENTION.MEDIUM];

return (
<section
data-position={position}
className={classnames(STYLE.wrapper, className)}
style={{ ...style, zIndex: zIndex }}
aria-label={ariaLabel}
>
<ToastContainer
{...commonProps}
position={position}
limit={limit}
containerId={getContainerID(id, attentionOrder[0])}
/>
<ToastContainer
{...commonProps}
position={position}
limit={limit}
containerId={getContainerID(id, attentionOrder[1])}
/>
</section>
<>
<section
data-position={position}
className={classnames(STYLE.wrapper, className)}
style={{ ...style, zIndex: zIndex }}
aria-label={ariaLabel}
>
<ToastContainer
{...commonProps}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this react-toastify ToastContainer is giving the role="alert" to that wrapper (with empty "" aria-label !). Role="alert" could interfere with our new SRAnnouncement. So let's pass role: "generic" to commonProps so that the DOM gets no role="alert" wrapper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(not doing, as discussed)

Copy link
Collaborator

@nataliadelmar nataliadelmar Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oki, we are doing this later so that non IMC notifications are still announced with role="alert"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, or maybe it won't be needed when you do your PR?

position={position}
limit={limit}
containerId={getContainerID(id, attentionOrder[0])}
/>
<ToastContainer
{...commonProps}
position={position}
limit={limit}
containerId={getContainerID(id, attentionOrder[1])}
/>
</section>
<ScreenReaderAnnouncer identity={id} />
</>
);
};

Expand Down
9 changes: 7 additions & 2 deletions src/components/NotificationSystem/NotificationSystem.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ type CustomOptions = {
*
* Can be used to trigger multiple notifications at different positions on the screen at the same time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add "also used to find which ScreenReader identity to send the SR Announcement to, in case screenReaderAnnouncement is defined"

Copy link
Collaborator Author

@gabrielchl gabrielchl Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i'm not sure this is needed, users of this component don't need to know the inner workings of the SR part

*/
notificationSystemId?: string;
notificationSystemId: string;
/**
* Screen reader announcement to be made.
* No announcement will be made if this is not provided.
*/
screenReaderAnnouncement?: string;
};

export type UpdateOptionsType = UpdateOptions & CustomOptions;
Expand Down Expand Up @@ -97,7 +102,7 @@ export interface Props {
/**
* Custom id for overriding this component's CSS.
*/
id?: string;
id: string;

/**
* Custom style for overriding this component's CSS.
Expand Down
34 changes: 27 additions & 7 deletions src/components/NotificationSystem/NotificationSystem.unit.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ describe('<NotificationSystem />', () => {

act(() => {
NotificationSystem.notify(
<NotificationTemplate aria-label="Some notification" content={content} toastCloseButtonLabel="Close notification" />,
<NotificationTemplate
aria-label="Some notification"
content={content}
toastCloseButtonLabel="Close notification"
/>,
{
toastId: fixedToastId,
notificationSystemId: id,
Expand Down Expand Up @@ -364,15 +368,20 @@ describe('<NotificationSystem />', () => {
it('should show a notification after notify has been fired and disappears after dismiss has been fired', async () => {
expect.assertions(4);

render(<NotificationSystem ariaLabel="test" />);
render(<NotificationSystem id="id" ariaLabel="test" />);

const toastId = '12345';
act(() => {
NotificationSystem.notify(
<NotificationTemplate aria-label="Some label" content={textContent} toastCloseButtonLabel="Close notification" />,
<NotificationTemplate
aria-label="Some label"
content={textContent}
toastCloseButtonLabel="Close notification"
/>,
{
autoClose: false,
toastId,
notificationSystemId: 'id',
}
);
});
Expand All @@ -398,7 +407,7 @@ describe('<NotificationSystem />', () => {
expect.assertions(5);
const user = userEvent.setup();

render(<NotificationSystem ariaLabel="test" />);
render(<NotificationSystem id="id" ariaLabel="test" />);

const closeButtonText = 'Close';
const toastId = '12345';
Expand All @@ -414,6 +423,7 @@ describe('<NotificationSystem />', () => {
autoClose: false,
toastId,
attention: NotificationSystem.ATTENTION.MEDIUM,
notificationSystemId: 'id',
}
);
});
Expand All @@ -437,16 +447,21 @@ describe('<NotificationSystem />', () => {
});

it('should update an existing notification', async () => {
render(<NotificationSystem ariaLabel="test" />);
render(<NotificationSystem id="id" ariaLabel="test" />);

const toastId = '12345';
const newcontent = 'this is a new text';
act(() => {
NotificationSystem.notify(
<NotificationTemplate aria-label="Some label" content={textContent} toastCloseButtonLabel="Close notification" />,
<NotificationTemplate
aria-label="Some label"
content={textContent}
toastCloseButtonLabel="Close notification"
/>,
{
autoClose: false,
toastId,
notificationSystemId: 'id',
}
);
});
Expand All @@ -461,8 +476,13 @@ describe('<NotificationSystem />', () => {
act(() => {
NotificationSystem.update(toastId, {
render: (
<NotificationTemplate aria-label="Some label" content={newcontent} toastCloseButtonLabel="Close notification" />
<NotificationTemplate
aria-label="Some label"
content={newcontent}
toastCloseButtonLabel="Close notification"
/>
),
notificationSystemId: 'id',
});
});

Expand Down
Loading
Loading