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.

Sign up for GitHub

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

Jump to bottom

[docs] Remove trailing slash from the URLs #748

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+7 −6
Open

[docs] Remove trailing slash from the URLs #748

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f3167d
Remove trailing slash in URLs
vladmoroz Oct 18, 2024
977039e
Update broken link checks
vladmoroz Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/next.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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
Member

Choose a reason for hiding this comment

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

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.

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 •
edited
Loading

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

env: {
// docs-infra
LIB_VERSION: rootPackage.version,
Expand Down
Loading