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

Jump to bottom

fix(theme): add aria-label to skip to content link region #7982

Merged
merged 6 commits into from
Aug 24, 2022
Merged

fix(theme): add aria-label to skip to content link region #7982

merged 6 commits into from
Aug 24, 2022

Conversation

YoniChechik
Copy link
Contributor

@YoniChechik YoniChechik commented Aug 21, 2022 •
edited
Loading

fixes #7981

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 21, 2022
Josh-Cena
Josh-Cena requested changes Aug 21, 2022
View reviewed changes
packages/docusaurus-theme-classic/src/theme/SkipToContent/index.tsx Outdated
@@ -14,7 +14,7 @@ import styles from './styles.module.css';
export default function SkipToContent(): JSX.Element {
const {containerRef, handleSkip} = useSkipToContent();
return (
<div ref={containerRef} role="region">
<div ref={containerRef} role="region" aria-label="Skip to main content">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a translation? It would share the same key as the main content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Aug 21, 2022
@Josh-Cena Josh-Cena changed the title Accecability problem in skip to main content div role="region" fix: add aria-label to skip to content link region Aug 21, 2022
@netlify
Copy link

netlify bot commented Aug 21, 2022 •
edited
Loading

✅ [V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 818d123
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6302857b707e240008db8174
😎 Deploy Preview https://deploy-preview-7982--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 Aug 21, 2022 •
edited
Loading

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 91 🟢 98 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟠 84 🟢 100 🟢 100 🟢 100 🟢 90 Report

Josh-Cena
Josh-Cena requested changes Aug 21, 2022
View reviewed changes
packages/docusaurus-theme-classic/src/theme/SkipToContent/index.tsx Outdated
Comment on lines 17 to 21
<div ref={containerRef} role="region" aria-label={<Translate
id="theme.common.skipToMainContentAriaLabel"
description="The skip to content aria label used for accessibility of the region div ">
Skip to main content
</Translate>}>
Copy link
Collaborator

@Josh-Cena Josh-Cena Aug 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You should use translate. JSX elements are objects, not strings. You can check the other aria-labels:

docusaurus/packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/CollapseButton/index.tsx

Lines 25 to 29 in 94067ce

aria-label={translate({
id: 'theme.docs.sidebar.collapseButtonAriaLabel',
message: 'Collapse sidebar',
description: 'The title attribute for collapse button of doc sidebar',
})}

It also needs to be formatted. If you don't want to check this out locally and run format, I can do that for you later.

Also, no need to add another id; just reuse the exact same thing from the <Translate> element. translate({ id: "theme.common.skipToMainContent" }) should be sufficient already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. Thanks for the help. I'm not really a JS pro...

@Josh-Cena
Copy link
Collaborator

Okay, it's good from my side. I would check this out locally and if it works as expected we can merge it.

@YoniChechik YoniChechik requested review from Josh-Cena and removed request for slorber and lex111 August 22, 2022 14:14
slorber
slorber approved these changes Aug 24, 2022
View reviewed changes
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.

LGTM thanks

@slorber slorber changed the title fix: add aria-label to skip to content link region fix(theme): add aria-label to skip to content link region Aug 24, 2022
@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Aug 24, 2022
@slorber slorber merged commit c2ce8a0 into facebook:main Aug 24, 2022
slorber pushed a commit that referenced this pull request Sep 1, 2022
@YoniChechik @Josh-Cena @slorber
fix(theme): add aria-label to skip to content link region (#7982)
41aa801 
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
@YoniChechik YoniChechik deleted the patch-1 branch September 8, 2022 06:58
@slorber slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Sep 28, 2022
@slorber slorber mentioned this pull request Oct 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorber slorber slorber approved these changes

@Josh-Cena Josh-Cena Awaiting requested review from Josh-Cena Josh-Cena is a code owner

Assignees
No one assigned
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accecability problem in <div role=region"><a class="skipToContent">
4 participants
@YoniChechik @Josh-Cena @slorber @facebook-github-bot