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

refactor: normalize Markdown linkification behavior, elaborate in documentation #7248

Merged
merged 6 commits into from
Apr 29, 2022

Conversation

Josh-Cena
Copy link
Collaborator

Motivation

Fix #7247.

This change introduces a little breaking change: absolute links like /otherFolder/doc.md is never resolved against the current file's location, and relative links like ./folder/doc.md is never resolved against the content root (should be rare, because it was never documented)

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Added a test case

@Josh-Cena Josh-Cena added pr: documentation This PR works on the website or other text documents in the repo. pr: polish This PR adds a very minor behavior improvement that users will enjoy. labels Apr 27, 2022
@Josh-Cena Josh-Cena requested a review from lex111 as a code owner April 27, 2022 03:11
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 27, 2022
@@ -9,4 +9,4 @@
- [doc1](doc1.md)
- [doc2](./doc2.md)

- [doc-localized](./doc-localized.md)
- [doc-localized](/doc-localized.md)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is intended. This file is actually not in the localized directory, so relative links shouldn't magically point to the fr localized directory. Changing it to an absolute link will cause the fr directory to be matched in priority.

@@ -4,7 +4,7 @@ Plugins are the building blocks of features in a Docusaurus 2 site. Each plugin

## Creating plugins {#creating-plugins}

A plugin is a function that takes two parameters: `context` and `options`. It returns a plugin instance object (or a promise). You can create plugins as functions or modules. For more information, refer to the [plugin method references section](./api/plugin-methods/README.md).
A plugin is a function that takes two parameters: `context` and `options`. It returns a plugin instance object (or a promise). You can create plugins as functions or modules. For more information, refer to the [plugin method references section](../api/plugin-methods/README.md).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a typo. It should have been caught because it doesn't make much sense!

@netlify
Copy link

netlify bot commented Apr 27, 2022

[V2]

Name Link
🔨 Latest commit 3a131b4
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/626b4f4520a1880008e356f9
😎 Deploy Preview https://deploy-preview-7248--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 Apr 27, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 64
🟢 Accessibility 100
🟠 Best practices 83
🟢 SEO 100
🟢 PWA 90

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

@github-actions
Copy link

github-actions bot commented Apr 27, 2022

Size Change: +20 B (0%)

Total Size: 803 kB

ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 50.1 kB +2 B (0%)
website/build/assets/css/styles.********.css 107 kB 0 B
website/build/assets/js/main.********.js 607 kB +18 B (0%)
website/build/index.html 38.8 kB 0 B

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Apr 27, 2022

@Josh-Cena 👍 to resolve against version content folders absolute paths.

As far as I understand it, now in the ops repo, [Test link](/tutorial-basics/quickstart/quickstart.md) would work.

I think we can try to avoid the breaking change and simply resolve md links a bit more aggressively?

IMHO it never makes sense for a website to end-up with links ending with .md or .mdx so whenever we find a way to resolve these paths to a slug, even if it looks suspicious and less portable, we probably want to do so? Not everyone need versioning/i18n

Also, I think resolving links from the root may make sense for GitHub linking?

I think we keep current logic and just add more resolution source folders, so that both of those work?

[Test link](/docs/tutorial-basics/quickstart/quickstart.md)
[Test link](/tutorial-basics/quickstart/quickstart.md)

And eventually, we could allow users to later configure extra resolution folders so that this is also possible?

[Test link](/repo-subdir/website/docs/tutorial-basics/quickstart/quickstart.md)

That would be less fail-fast for docs portability (ie user will know it's bad linking only after trying to version or use i18n) but would also improves linking DX overall.

In general, link portability is a broad term, it could as well mean Docusaurus single version + GitHub, or Docusaurus versioning + i18n.

I think there are also some opportunities to automatically fix the link portability problem with some CLI tool.

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Apr 27, 2022

I'm open to add links that resolve against the site dir, but the real absolute path (/Users/JoshCena/...) seems absurd to me.

I have had many similar requests in the past where people want to create links that map to an external URL (e.g. #6463). When we have a dedicated markdown config and a global Markdown infrastructure, I'd like to create a linkMapper config so you can do linkMapper: { "<REPO_ROOT>": "https://github.com/org/repo" } and then write [link](<REPO_ROOT>/file.md). But for now let's keep it simple and reasonable

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 👍

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: documentation This PR works on the website or other text documents in the repo. pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting broken link on existing document path
3 participants