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(v1): add 'slugPreprocessor' config option to allow users customize the hash links #3124

Merged
merged 3 commits into from
Jul 28, 2020

Conversation

Simek
Copy link
Contributor

@Simek Simek commented Jul 25, 2020

Motivation

Refs facebook/react-native-website#2081

Currently in V1 HTML content from the headers is a part of hash links which in some cases can make the links non-intuitive or obfuscate them for the users.

Note: At the beginning on the React Native website labels were using only pseudo classes, but because of accessibility reasons the content has been moved to the tag, which made this issue even worse.

This PR fixes the reported issue by replacing all the HTML content (tags and text content inside) using RegExp before the header slug generation.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I have to update the result of two existing test, but the RegExp works well so the does not strip tags randomly test do not need the change.

I have also tested the changes locally with the react-native-website repository.

Examples

Link before the changes: /docs/next/button#nextfocusforward-div-classlabel-androidandroiddivdiv-classlabel-tvtvdiv
Link after the changes: /docs/next/button#nextfocusforward

Link before the changes: /docs/next/button#div-classlabel-required-basicrequireddivonpress
Link after the changes: /docs/next/button#onpress

Related PRs

WIP - I need to check the V1 docs.

I was not able to find the information about slug generation or hash links in the V1 docs. Correct me if I have missed something.

After the rewrite as a new feature the additional config documentation entry were required so I have updated the documentation page within this PR.

@Simek Simek requested a review from yangshun as a code owner July 25, 2020 14:09
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 25, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jul 25, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 159c152

https://deploy-preview-3124--docusaurus-2.netlify.app

@slorber slorber added the v1 This issue is for Docusaurus 1 label Jul 27, 2020
@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

Hi,

I'm not very familiar with v1, but this looks to me like a non-retrocompatible change that is likely to break some existing anchor-based linking on existing v1 sites right?

Even if the behavior is currently not ideal, I'm not sure we want to change the anchor slug generation behavior at this point where we try to focus on v2 and help on v1->v2 migrations

@Simek
Copy link
Contributor Author

Simek commented Jul 27, 2020

Hi @slorber, there is a lot of issues with React Native website migration to V2 and solving them will require a lot effort and time on our side (I have been talking about the process with @yangshun). So in a meanwhile we decided to fix most urgent issues that our community is reporting with V1. And yes, this change could be seen as a breaking change for some users.

@yangshun
Copy link
Contributor

@Simek please involve @slorber actively moving forward, he's the main guy on Docusaurus now :)

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

As discussed on discord, I'd prefer if you provide a solution to this problem that is not a breaking change for all users, particularly when broken links will likely be unnoticed on v1 site upgrades because it will not crash the build.

You can use patch-package to provide custom slug behavior on your site, or try to submit an opt-in way to change the anchor links behavior, which would be non-breaking change by default.

@Simek Simek changed the title fix(v1): remove HTML content from hash links feat(v1): add 'slugPreprocessor' config option to allow users customize the hash links Jul 27, 2020
@slorber
Copy link
Collaborator

slorber commented Jul 28, 2020

Thanks, this is retrocompatible so it's safe to merge

@slorber slorber merged commit aa7430e into facebook:master Jul 28, 2020
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Jul 29, 2020
@slorber
Copy link
Collaborator

slorber commented Aug 1, 2020

released in https://github.com/facebook/docusaurus/releases/tag/v1.14.5

please let me know if this works, as it's my first v1 release 😅

@Simek
Copy link
Contributor Author

Simek commented Aug 1, 2020

@slorber It looks like there was no NPM publish for 1.14.5 version:

@slorber
Copy link
Collaborator

slorber commented Aug 2, 2020

oh sorry!

should be better now

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. v1 This issue is for Docusaurus 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants