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

fix(v2): classic theme - semantic correct anchors links #5080

Merged
merged 2 commits into from
Jun 30, 2021

Conversation

AuHau
Copy link
Contributor

@AuHau AuHau commented Jun 29, 2021

Motivation

Across the Classic Theme, there are two places where a link points to an anchor that does not exist on the page. This is for example problem for tools that try to verify the validity of links and if applicable their anchors. These tools are crucial in maintaining coherent documentations.

This PR fixes these links to be semantic correct and uses simple # instead.

Alternative to this change is to introduce #main anchor for the "Skip to main content" on the main-wrapper element. But not sure what would then be introduced for the Doc Sidebar.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Related PRs

None

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 29, 2021
@netlify
Copy link

netlify bot commented Jun 29, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: ddd18fe

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60dc306ec43c1c00076605d6

😎 Browse the preview: https://deploy-preview-5080--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 29, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 91
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5080--docusaurus-2.netlify.app/

@slorber
Copy link
Collaborator

slorber commented Jun 29, 2021

These tools are crucial in maintaining coherent documentations.

I don't have much experience with those external tools but it could be nice for Docusaurus to provide some recommendations about that (ideally we should also detect broken anchor links, another feature request). Can you share your experience?


@lex111 any opinion about removing #! and #main? I don't remember exactly why we have those anchors

@slorber
Copy link
Collaborator

slorber commented Jun 29, 2021

Apparently this leads to Lint errors:

https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md

/home/runner/work/docusaurus/docusaurus/packages/docusaurus-theme-classic/src/theme/DocSidebar/index.tsx
Error:   131:7  error  Anchor used as a button. Anchors are primarily expected to navigate. Use the button element instead. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid

/home/runner/work/docusaurus/docusaurus/packages/docusaurus-theme-classic/src/theme/SkipToContent/index.tsx
Error:   42:7  error  Anchor used as a button. Anchors are primarily expected to navigate. Use the button element instead. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid

@AuHau
Copy link
Contributor Author

AuHau commented Jun 29, 2021

I don't have much experience with those external tools but it could be nice for Docusaurus to provide some recommendations about that (ideally we should also detect broken anchor links, another feature request). Can you share your experience?

Well, I am just exploring this space myself. We have generated API reference from Typedoc where we have lot of links for our upstream documentation (external links). I can foresee moments where the upstream docs will change and I expect 99% chance that we won't detect that, so I am trying to come up with some automatic detection for that. I found https://github.com/timaschew/link-checker that looks like a good tool for that task.

@lex111 any opinion about removing #! and #main? I don't remember exactly why we have those anchors

Apparently this leads to Lint errors:

What I found during my research was this PR (related to the #main) #4162 and its linked issue #3917 that was doing exactly the opposite then the Linting rule suggests for accessibility reasons. So maybe I would suggest suppressing that rule for these occurrences? Or maybe it is not relevant anymore?

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.

Apparently, we can safely remove hashbang from sidebar links, because it is deprecated technique. I can not say exactly why was initially added.
And also I'm not against suppressing ESLint errors for skip link too.

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jun 30, 2021
@slorber
Copy link
Collaborator

slorber commented Jun 30, 2021

thanks for the feedback 👍

@slorber slorber merged commit 823b020 into facebook:master Jun 30, 2021
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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants