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

[docs] Remove trailing slash from the URLs #748

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

vladmoroz
Copy link
Contributor

No description provided.

@vladmoroz vladmoroz added the docs Improvements or additions to the documentation label Oct 18, 2024
@mui-bot
Copy link

mui-bot commented Oct 18, 2024

Netlify deploy preview

https://deploy-preview-748--base-ui.netlify.app/

Generated by 🚫 dangerJS against 977039e

@@ -20,6 +20,7 @@ const rootPackage = loadPackageJson();

/** @type {import('next').NextConfig} */
const nextConfig = {
trailingSlash: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure when I last saw a website with trailing slashes URLs or if I ever saw anyone manually type out a trailing slash in a link. Seems a bit old-school

@oliviertassinari do you remember why trailing slash is used across the board? Is there a hard reason it's still needed? I dug up this as the PR that introduced it originally

Copy link
Member

@oliviertassinari oliviertassinari Oct 18, 2024

Choose a reason for hiding this comment

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

do you remember why trailing slash is used across the board?

@vladmoroz Next.js < 9.0.0 would have trailing slashes by default. Because those are two different URLs, to retain the same SEO, we never changed the URLs.

Over time, we added logic to throw the docs anytime a link with a missing trailing slash was added. Those are not easy to spot but break SEO.

A progressive migrating to non trailing slash would make sense. We would need to handle it domain name per domain name, and so need docs-infra to support both (meaning to update the error detection logic to support the both). Going from one to the next mode likely is about batching with a domain name change. As far as I know, this change on its own isn't worth the SEO hit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, sounds like it's not a concern for the Base UI site then

Copy link
Member

@oliviertassinari oliviertassinari Oct 18, 2024

Choose a reason for hiding this comment

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

Comment updated.

sounds like it's not a concern for the Base UI site then

Yes and no.

  • Can Base UI remove trailing slash (this PR): yes, there is no existing SEO hit possible.
  • Does Base UI can not throw when using trailing slash: no, Netlify and Next.js have silent redirections. We need to break so contributors don't add regressions without realizing.
  • If Base UI hosts docs-infra, can it ignore trailing slashes support: no, we will likely only migrate Material UI, MUI X with a domain name change, e.g. mui.com/material-ui to material-ui.com or material-ui.newco.com. So need dual support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, thanks for the context. Now that I am seeing there is a broken link detection script in place, I updated it to account for no trailing slashes in these docs, so just Base UI itself should be good to go now.

Up next we can revisit trailing slashes when Material UI and X docs are moved to a new domain and/or when we do a proper docs-infra package.

Side note: I'd expect that docs-infra will be a standalone repo + npm package. It's not an intrinsic part of the Base UI product, it's just a company-wide concern across our projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

None of our products should have trailing slashes, they make us look unprofessional.

Copy link
Member

@oliviertassinari oliviertassinari Oct 20, 2024

Choose a reason for hiding this comment

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

Now that I am seeing there is a broken link detection script in place, I updated it to account for no trailing slashes in these docs, so just Base UI itself should be good to go now.

@vladmoroz this is a nice improvement 👍.

I think that we will also need https://github.com/mui/material-ui/blob/6679d01eed664b4949d495e2b0221b99a9fde966/packages/markdown/parseMarkdown.js#L82 soon or later, at least to migrate Material UI docs-infra. This (throwing with a clear error, right in dev mode) should be a much better DX than the script. As far as I remember, the script is a lifeline but it's hard to debug when making a mistake.

I have run a quick scan with https://www.screamingfrog.co.uk/seo-spider/. The only link issue he can find is with https://deploy-preview-748--base-ui.netlify.app/components/react-separator

SCR-20241021-bynq

those returns 404. It looks like a regression from this PR.

I'd expect that docs-infra will be a standalone repo + npm package. It's not an intrinsic part of the Base UI product, it's just a company-wide concern across our projects.

There is https://www.notion.so/mui-org/docs-infra-Code-location-c292d34ed5b0487a9ade846ef11d4019 on this topic specifically. As I see this, we have to pick between one of those two repositories: mui-public (to be renamed to newco-public) or base-ui. Historically, it's in material-ui and not in another repository so we can learn from changes very quickly. Without this, we have to do two extra steps:

  • have a solid test case in the other repository, something that reproduces most of the edge of downstream
  • propagate changes downstream daily or more frequently from that other repository to the others (forcing as few breaking changes as possible, and progressive migration paths as it would happen if in the actual repo)

No specific view on which one we should go with. I think it's whatever helps the docs-infra team the most.

How would we handle Base UI X Since the same content would be used both for Base UI X and MUI X, it will be hard to have trailing slashes on MUI X but not on Base UI X.

@flaviendelangle I don't think this will be a problem. We could have https://base-ui.com/x/react-data-grid and https://mui.com/x/react-data-grid/ powered by the same markdown file and demos files.

None of our products should have trailing slashes, they make us look unprofessional.

@colmtuite Yeah, I think there is an agreement that this is the right direction. It's rare to see any website do what's done in mui.com.

Copy link
Member

Choose a reason for hiding this comment

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

@flaviendelangle I don't think this will be a problem. We could have https://base-ui.com/x/react-data-grid and https://mui.com/x/react-data-grid/ powered by the same markdown file and demos files.

And when writting internal links in the md, we would always use one that would be transformed at build time if needs be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have run a quick scan with https://www.screamingfrog.co.uk/seo-spider/. The only link issue he can find is with https://deploy-preview-748--base-ui.netlify.app/components/react-separator

SCR-20241021-bynq

those returns 404. It looks like a regression from this PR.

These never worked btw, this demo renders <a href="."> which I'll fix when reworking the demos

Copy link
Member

@oliviertassinari oliviertassinari Nov 17, 2024

Choose a reason for hiding this comment

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

And when writting internal links in the md, we would always use one that would be transformed at build time if needs be?

@flaviendelangle This makes sense. I think in that world, we would write links without trailing slashes, and docs-infra would add them where needed. I believe Next.js has logic around this. This way, we default toward being close to what we aim to reach.

Issue created for the overall topic: mui/mui-public#248.

@vladmoroz vladmoroz changed the title Remove trailing slash from the URLs [docs] Remove trailing slash from the URLs Oct 22, 2024
@vladmoroz vladmoroz merged commit ee987b6 into mui:master Oct 23, 2024
18 checks passed
@vladmoroz vladmoroz deleted the trailing-slash branch October 23, 2024 19:26
atomiks pushed a commit to atomiks/base-ui that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants