Skip to content

Conversation

@DiegoAndai
Copy link
Member

Fix broken links reported in https://app.circleci.com/pipelines/github/mui/base-ui/2800/workflows/addeece5-c28e-48b7-9f29-fb49d9bf1327/jobs/17721.

All of these are from core to core, so the link check broke. It would be good to fix this check in this same PR.

@DiegoAndai DiegoAndai added type: bug It doesn't behave as expected. docs Improvements or additions to the documentation. labels Aug 1, 2024
@DiegoAndai DiegoAndai self-assigned this Aug 1, 2024
@DiegoAndai
Copy link
Member Author

I'm curious why the check reports links on https://mui.com/ domain, as most of these changes are on next only, and as far as I can tell they haven't leaked into master.

@alexfauquette
Copy link
Member

I'm curious why the check reports links on https://mui.com/ domain, as most of these changes are on next only, and as far as I can tell they haven't leaked into master.

Because it does not really look at https://mui.com it's just a sugar syntax in the report such that you can easily copy past the URL to see the page in your browser:

.forEach((linkKey) => {
write(`- https://mui.com${linkKey}`);
console.log(`https://mui.com${linkKey}`);
console.log(`used in`);
usedLinks[linkKey].forEach((f) => console.log(`- ${path.relative(docsSpaceRoot, f)}`));

The true testing is about relative links

.filter((link) => link.startsWith('/'))

@mnajdova
Copy link
Member

mnajdova commented Aug 2, 2024

Because it does not really look at https://mui.com it's just a sugar syntax in the report such that you can easily copy past the URL to see the page in your browser:

It looks like a good candidate to use the <!-- #default-branch-switch --> comment, we can switch it when changing the default branch.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged. and removed PR: out-of-date The pull request has merge conflicts and can't be merged. labels Aug 2, 2024
@michaldudak
Copy link
Member

These changes look OK, but IMO it would be better to fix the link-check script first (to ensure it catches them)

@alexfauquette
Copy link
Member

The fix is here #43195

The script was completely broken due to what I assume is a breaking change in the marked package. The scrip was unable to extract links from pages.

@michaldudak Do you know if it's feasible to add test on scripts? None of the docs scripts are tested. I don't know what to do to add one such that this does not happened again

@michaldudak
Copy link
Member

The cleanest way I can think of would be to extract the script logic into packages-internal/scripts, test it there and leave just the CLI in the scripts directory.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 16, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 23, 2024
@DiegoAndai
Copy link
Member Author

Hey @alexfauquette, sorry for the delay on this PR. I updated it, and it's ready for review again.

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

As long as the section anchor exist, I'm happy with it ;)

@DiegoAndai DiegoAndai merged commit e025f80 into mui:master Aug 26, 2024
@DiegoAndai DiegoAndai deleted the fix-broken-links branch August 26, 2024 15:42
<p class="blog-description"><a href="https://codesandbox.io/p/sandbox/stupefied-mclaren-ho4zs?file=/src/App.tsx">CodeSandbox</a></p>

**Second**, you can add [custom variants](/material-ui/customization/theme-components/#creating-new-component-variants) to the theme, overriding the CSS for specific component prop combinations.
**Second**, you can add [custom variants](https://v5.mui.com/material-ui/customization/theme-components/#creating-new-component-variants) to the theme, overriding the CSS for specific component prop combinations.
Copy link
Member

Choose a reason for hiding this comment

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

Interesting tradeoff. A blog post like v5 isn't read much in the future, the page view distribution overtime is very much an exp(2-2x), so we shouldn't have to maintain those links 👍

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. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants