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): do not process anchor links by router #2580

Merged
merged 1 commit into from
Apr 11, 2020
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Apr 11, 2020

Motivation

Resolve #2552

Anchor links that refer to sections in the current file should not be processed by the router, because otherwise the current URL is added to them ("/docs/docs/#doc" instead of just "#doc"), and this breaks them (in case of non-ASCII characters).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

In any MD document, define new heading using non-ASCII characters. And then try to refer to
this section through a new link.

The example below uses Russian characters, for example, but you can use Persian, as indicated in #2552

...

# Тест

...

[Тест](#тест)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Apr 11, 2020
@lex111 lex111 added this to the v2.0.0-alpha.51 milestone Apr 11, 2020
@lex111 lex111 requested a review from yangshun as a code owner April 11, 2020 10:16
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 11, 2020
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 95acb6f

https://deploy-preview-2580--docusaurus-2.netlify.com

@lex111
Copy link
Contributor Author

lex111 commented Apr 11, 2020

@yangshun I wonder why we use NavLink instead of regular Link component for Docusaurus/Link component? This is rather strange, considering that NavLink is needed only in NavBar and Footer components, and in regular (or user-land) links, the functionality inherent in NavLink is not needed.

From docs:

A special version of the that will add styling attributes to the rendered element when it matches the current URL.

As a result, all internal links have extra attributes (aria-current="page" class="active"):

... <a aria-current="page" class="active" href="/docs/docs/#sidebar-item">sidebar item</a>

... <a aria-current="page" class="active" href="/docs/cli">here</a>

In the case of anchors links now this will not happen (but still relevant for other internal links, this is an unnecessary job that makes the router).

I suggest replacing it with Link component, any objections against this?

@yangshun
Copy link
Contributor

I suggest replacing it with Link component, any objections against this?

Yep, I think this makes sense.

@yangshun yangshun merged commit 476de09 into master Apr 11, 2020
@yangshun yangshun deleted the lex111/iss-2552 branch April 11, 2020 15:00
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.

In markdown, Link to the content doesn't work when the link name is Unicode
4 participants