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] Redirect NoSsr, Portal and TextareaAutosize to Base UI API page #37175

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented May 5, 2023

Fixes issue mentioned in comment #37121 (comment)

@ZeeshanTamboli ZeeshanTamboli added docs Improvements or additions to the documentation core Infrastructure work going on behind the scenes regression A bug, but worse labels May 5, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [docs] Redirect to Base UI API [docs] Redirect NoSsr, Portal and TextareaAutosize to Base UI API page May 5, 2023
@mui-bot
Copy link

mui-bot commented May 5, 2023

Netlify deploy preview

https://deploy-preview-37175--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against a45d521

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 5, 2023 12:55
@ZeeshanTamboli ZeeshanTamboli added the package: material-ui Specific to @mui/material label May 5, 2023
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

  1. The URL is incorrect. https://deploy-preview-37175--material-ui.netlify.app/material-ui/react-no-ssr/#api links to https://deploy-preview-37175--material-ui.netlify.app/base/api/no-ssr/ which is a 301. It's better than a 404 but it's not correct. It degrades the UX, and wastes some SEO crawl budget, e.g.
Screen.Recording.2023-05-06.at.22.06.14.mov

  1. We got a 2-3 regressions around the links, this is maybe worth a test case.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 8, 2023

@oliviertassinari

  1. The URL is incorrect. https://deploy-preview-37175--material-ui.netlify.app/material-ui/react-no-ssr/#api links to https://deploy-preview-37175--material-ui.netlify.app/base/api/no-ssr/ which is a 301. It's better than a 404 but it's not correct. It degrades the UX, and wastes some SEO crawl budget, e.g.

It at-least is better than adding the API doc manually. If we add the docs manually, the API list can get outdated easily. Anyway, we are going to remove these components from Material UI in the next major release.

  1. We got a 2-3 regressions around the links, this is maybe worth a test case.

Added tests.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks! Agree, we should fix these urls to point to the correct pages (not use redirects). We probably will need to store somewhere the data about where the API page is shown (we can't create it based on the component name anymore, as the docs lays on some documentation page).

@ZeeshanTamboli ZeeshanTamboli merged commit 3059bd6 into mui:master May 8, 2023
@ZeeshanTamboli ZeeshanTamboli deleted the add-more-components-in-base-ui-re-exoported-components branch May 8, 2023 10:25
test(`should have correct API link when linking Base UI component ${component}`, async ({
page,
}) => {
await page.goto(`/material-ui/react-${kebabCase(component || '')}/`);
Copy link
Member

@oliviertassinari oliviertassinari May 8, 2023

Choose a reason for hiding this comment

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

End to end tests are slow, why not write a test case on the text output of the markdown logic? It would be about x100 faster to run.

Suggested change
await page.goto(`/material-ui/react-${kebabCase(component || '')}/`);

@oliviertassinari
Copy link
Member

oliviertassinari commented May 8, 2023

will need to store somewhere the data about where the API page is shown (we can't create it

The markdown logic runs in a webpack plugin. I guess we could either:

  • write the links manually with markdown (there aren't that many cases that are broken like this, maybe 10, to check ahrefs crawl to find them all)
  • read the page file, with the API links
  • load the logic that generates the page file

binh1298 pushed a commit to binh1298/material-ui that referenced this pull request May 17, 2023
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 docs Improvements or additions to the documentation package: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants