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

[core] Avoid 301 links #8383

Merged
merged 2 commits into from Mar 31, 2023
Merged

[core] Avoid 301 links #8383

merged 2 commits into from Mar 31, 2023

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 25, 2023

This surfaces in https://app.ahrefs.com/site-audit/3523498/61/data-explorer?columns=pageRating%2Curl%2Ctraffic%2ChttpCode%2CcontentType%2Cdepth%2Credirect%2CincomingAllLinks&filterId=b3b75b6257a370fcf9f1f73befb5deb5&issueId=c64d8847-d0f4-11e7-8ed1-001e67ed4656&sorting=-pageRating.

It's a good opportunity to use a link to the base branch so that once we change the default branch, the links on the stable version of the docs stay correct.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Mar 25, 2023
@mui-bot
Copy link

mui-bot commented Mar 25, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8383--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 628 1,080.2 640.1 795.58 191.142
Sort 100k rows ms 594.7 1,399.7 1,399.7 1,026.52 257.546
Select 100k rows ms 215.2 389.9 221 265.86 67.235
Deselect 100k rows ms 171.2 278.9 194.1 212.92 37.508

Generated by 🚫 dangerJS against 9f338c5

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 27, 2023

We specifically have gone with the branch independent link so that we’d no longer need to update it once the default branch is changed…

@LukasTy My thought is that if the docs always point to HEAD, then once we make next the active branch (used by <10% of the sessions) then:

  1. if we change the translation, the developer on the stable docs will be confused as to why they don't receive the same value.
  2. if we change the location of the translations, the developer on the stable docs would have 404 links.

What is the issue with updating the links when the default branch changes?
In the case of scripts/l10n.ts it's here for free, there is nothing more to replace it.
I would expect that using #default-branch-switch to find the 2-3 patterns to global search and replace, and do the search and replace to be enough.

Now, it could be even better to have a markdown logic that replaces the links to use the version tag. https://github.com/mui/mui-x/blob/-/packages/grid/x-data-grid/src/models/gridSlotsComponentsProps.ts in the source could turn into https://github.com/mui/mui-x/blob/v6.0.3/packages/grid/x-data-grid/src/models/gridSlotsComponentsProps.ts in the HTML output for the cases where we don't want to show the latest stable/unstable version of the file. e.g. in

Screenshot 2023-03-28 at 01 25 31

not pointing to the latest stable/unstable version might be worse.

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

My thought is that if the docs always point to HEAD, then once we make next the active branch (used by <10% of the sessions) then:

  1. if we change the translation, the developer on the stable docs will be confused as to why they don't receive the same value.
  2. if we change the location of the translations, the developer on the stable docs would have 404 links.

What is the issue with updating the links when the default branch changes? In the case of scripts/l10n.ts it's here for free, there is nothing more to replace it. I would expect that using #default-branch-switch to find the 2-3 patterns to global search and replace, and do the search and replace to be enough.

Fair points. We probably did not think about these issues that would arise when we work on the next major. 👌
There are definitely little to no issues in changing the needed places manually. 👍 😉

@alexfauquette alexfauquette merged commit 39d105c into mui:master Mar 31, 2023
16 checks passed
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 31, 2023

I have reverted the change to the link in the docs in 9f338c5 so we can benefit from mui/material-ui#36729.

@oliviertassinari oliviertassinari deleted the avoid-301 branch March 31, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants