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] Fix experimental API page duplication #35213

Merged

Conversation

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work docs Improvements or additions to the documentation labels Nov 20, 2022
@mui-bot
Copy link

mui-bot commented Nov 20, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35213--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against 1d9db5b

Comment on lines 280 to 282
{ pathname: '/guides/flow' },
],
},
Copy link
Member Author

Choose a reason for hiding this comment

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

I was tempted to remove all the rest, but I didn't to keep this PR relatively small.

Comment on lines -381 to -382
/joy-ui/core-features/perfect-dark-mode/ /joy-ui/main-features/perfect-dark-mode/ 301
/:lang/joy-ui/core-features/perfect-dark-mode/ /:lang/joy-ui/main-features/perfect-dark-mode/ 301
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a double redirection. it was added in https://github.com/mui/material-ui/pull/34613/files#diff-7a0cbac74402caf0e99ba338a9e7c42684580a1f85e149110c3c2ca6e38055afR390 @samuelsycamore A tip, when adding a new redirection, it's a good practice to also update the redirections that might already point to the new "old URL". It avoids a double 301.

Comment on lines 397 to +398
/joy-ui/guides/applying-dark-mode/ /joy-ui/customization/dark-mode/ 301
/:lang/joy-ui/guides/applying-dark-mode/ /:lang/joy-ui/customization/dark-mode/ 301
Copy link
Member Author

@oliviertassinari oliviertassinari Nov 20, 2022

Choose a reason for hiding this comment

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

Redirecting the en-US locale isn't enough.

Once we stop this generation of pt, zh pages, we will be able to not have /:lang/ for the redirection of pages first introduced after the generation stopped. So, it's not for tomorrow.

Comment on lines 224 to 227
{
pathname: '/material-ui/experimental-api/css-variables',
title: 'CSS variables',
},
Copy link
Member Author

@oliviertassinari oliviertassinari Nov 20, 2022

Choose a reason for hiding this comment

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

I didn't understand why https://mui.com/material-ui/experimental-api/css-variables/ isn't linked in the side nav of the docs.

If it's because https://mui.com/material-ui/experimental-api/css-theme-variables/overview is meant to replace it, then I think there could be a follow-up to:

Copy link
Member

Choose a reason for hiding this comment

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

From what I remember, initially I added one page for the CSS variables under the Experimental API section. After this, @siriwatknp updated it to include more pages, with usage, tutorial etc, and this is why this page is no longer in the side nav. I guess we can copy what we feel is useful in one of the new pages and drop this page. @siriwatknp what are your thoughts?

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 30, 2022

Choose a reason for hiding this comment

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

Right OK. I'm renaming the page to include "legacy" and will merge. The changes here are still a step forward, and the docs are experimental. We can further cleanup later on.

@oliviertassinari oliviertassinari added the scope: docs-infra Specific to the docs-infra product label Nov 30, 2022
@oliviertassinari oliviertassinari enabled auto-merge (squash) November 30, 2022 16:48
@oliviertassinari oliviertassinari merged commit 3751f20 into mui:master Nov 30, 2022
@oliviertassinari oliviertassinari deleted the clean-up-from-docs-split branch November 30, 2022 16:49
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@@ -1,4 +1,4 @@
# CSS variables
# TODO merge with other pages
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Jan 27, 2023

Choose a reason for hiding this comment

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

@oliviertassinari I see this in https://mui.com/material-ui/experimental-api/css-variables/. I think your intent was to add a comment here. As for the fix, should we remove this page or keep the heading as CSS variables?

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 29, 2023

Choose a reason for hiding this comment

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

I think that the next step is to merge the two pages. https://mui.com/material-ui/experimental-api/css-variables/ is superior in a couple of ways:

  1. it has an API reference page. https://mui.com/material-ui/experimental-api/css-variables/#api, e.g. it's the only search result for disableTransitionOnChange
Screenshot 2023-05-23 at 16 56 48
  1. it has two demos while there is not a single demo on the new pages. I would have expected many simple demos. So developers can play with it and understand what it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants