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

feat(core): route announcer #7074

Closed
wants to merge 27 commits into from
Closed

feat(core): route announcer #7074

wants to merge 27 commits into from

Conversation

seyoon20087
Copy link
Contributor

@seyoon20087 seyoon20087 commented Mar 30, 2022

Motivation

By doing this, client-side route changes will be announced to screen readers, improving accessibility to screen-reader users.

This was heavily inspired by https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/navigation.js#L173-L202.

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

  1. Add the route announcer component
  2. Include the route announcer component in the root

Related PRs

N/A

By doing this, client-side route changes will be announced to screen readers, improving accessibility to screen-reader users.

This was heavily inspired by https://github.com/vercel/next.js/blob/canary/packages/next/client/route-announcer.tsx.

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 30, 2022
@netlify
Copy link

netlify bot commented Mar 30, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 7d67a48
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/628dcf2b15c35e00083b43b0
😎 Deploy Preview https://deploy-preview-7074--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Mar 30, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 71 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 84 🟢 99 🟢 100 🟢 100 🟢 90 Report

I have dived in the codebase, and it looks like it has a 1000 second wait time before a new route can load.

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Someone suggested to put it in App.tsx instead of `@theme/Root`.

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
In my testing 50ms is ok to me. You may change it if it something goes wrong.

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
@seyoon20087 seyoon20087 requested a review from Josh-Cena March 30, 2022 13:19
@Josh-Cena Josh-Cena changed the title Add a route announcer feat(core): route announcer Mar 30, 2022
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Mar 30, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Thanks, seems to work great 👍 Will leave it to others to review as well. Left some more suggestions.

packages/docusaurus/package.json Outdated Show resolved Hide resolved
packages/docusaurus/src/client/routeAnnouncer.tsx Outdated Show resolved Hide resolved
packages/docusaurus/src/client/routeAnnouncer.tsx Outdated Show resolved Hide resolved
Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Someone suggested to this to adjust some code and to prevent
blocked on an external dep as commented.

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
yarn.lock Outdated Show resolved Hide resolved
seyoon20087 and others added 4 commits March 30, 2022 23:29
Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
@Josh-Cena Josh-Cena added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Mar 31, 2022
@Josh-Cena
Copy link
Collaborator

The timeout unfortunately doesn't work if the network is slow and the bundle preloading takes more than 50ms. I think this should better be implemented as a client module since it seems pretty decoupled from the main app. We can do this after fixing #6732

@Josh-Cena Josh-Cena marked this pull request as draft March 31, 2022 07:52
@Josh-Cena Josh-Cena removed the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Apr 3, 2022
@Josh-Cena Josh-Cena added status: awaiting review This PR is ready for review, will be merged after maintainers' approval and removed status: blocked This issue is blocked by another issue or external dep and can't be pushed further. labels Apr 3, 2022
@Josh-Cena Josh-Cena marked this pull request as ready for review April 3, 2022 14:12
…-- similarly to gatsby 4

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

The current behavior looks cool to me. I still somehow believe this can be factored out into a separate client module and not in the main component tree (since all it does is consuming the location and portaling to a DOM element), but I'll let @slorber take a look first. At least now we can be unblocked from #6732

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Perhaps we should move this component to the classic-theme area to make related message translatable?

@Josh-Cena
Copy link
Collaborator

I agree. Given the current architecture, it seems feasible to put this in layout as well.

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Hey, looks like an interesting addition.

I don't know how route announcement is supposed to be implemented theoretically, can you explain, please? Particularly the thing about using a custom element, a portal, rAF...

I don't understand a few choices in this PR that looks weird to me, but maybe I'm wrong

The announcement label template should rather be translatable, and this does not really belong in core

Maybe this feature could be a route announcement plugin using a client module?

packages/docusaurus/src/client/RouteAnnouncer.tsx Outdated Show resolved Hide resolved
packages/docusaurus/src/client/RouteAnnouncer.tsx Outdated Show resolved Hide resolved

export function Portal({
children,
type = 'docusaurus-portal',
Copy link
Collaborator

Choose a reason for hiding this comment

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

are other portal types planned?

What is the purpose of calling document.createElement("docusaurus-portal"); ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@slorber This is the container node's tag name. Portal elements can often be custom elements so as not to have any default user-agent styles.

packages/docusaurus/src/client/RouteAnnouncer.tsx Outdated Show resolved Hide resolved
packages/docusaurus/src/client/RouteAnnouncer.tsx Outdated Show resolved Hide resolved
packages/docusaurus/src/client/RouteAnnouncer.tsx Outdated Show resolved Hide resolved
packages/docusaurus/src/client/RouteAnnouncer.tsx Outdated Show resolved Hide resolved
@Josh-Cena Josh-Cena requested review from lex111 and slorber April 30, 2022 02:59
@Josh-Cena Josh-Cena marked this pull request as ready for review April 30, 2022 03:02
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

@slorber @lex111 I've move the announcer implementation to theme-classic so we can translate it. Because the client module has just been fixed, I decided to go with a pure-DOM solution without wiring it to a theme component, so that we have a use-case of reading and manipulating DOM in client module. I doubt if anyone wants to swizzle the announcement anyways. I've kept the Portal core component for now, although we don't use it anymore, in case it's useful in the future. Tell me what you think.

* LICENSE file in the root directory of this source tree.
*/

#docusaurus-route-announcer-content {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using an ID selector here. Not sure if we necessarily want a class name here, or if we want to use inline styles.

Comment on lines +20 to +28
const announcerContainer = document.createElement(
'docusaurus-route-announcer',
);
const announcement = document.createElement('p');
Object.assign(announcement, {
ariaLive: 'assertive', // Make the announcement immediately.
id: 'docusaurus-route-announcer-content',
role: 'alert',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pure DOM manipulation. Because client modules don't have access to the React context, it would be quite cumbersome if we want to use React to do this. The alternative will be to put this in Layout, but there may be other edge-cases (for example, the layout doesn't re-mount between doc pages)

@@ -49,6 +48,7 @@ const EXPECTED_CSS_MARKERS = [

// Lazy-loaded lib
'.DocSearch-Modal',
'.DocSearch-Hit-content-wrapper',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised that this moved, but it makes more sense this way? Not sure why it was loaded so early before.

Comment on lines +49 to +52
return dispatchLifecycleAction('onRouteDidUpdate', {
previousLocation,
location,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exposed the cleanup function here, so we can clean up the side-effect of appending DOM child.

@Josh-Cena Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label May 2, 2022
@seyoon20087 seyoon20087 closed this by deleting the head repository Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants