-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[base-ui][docs] Improve Next.js Link docs #39838
[base-ui][docs] Improve Next.js Link docs #39838
Conversation
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
<Button className="CustomButton">Button</Button> | ||
<Button className="CustomButton" disabled> | ||
<Button className="IntroductionButton">Button</Button> | ||
<Button className="IntroductionButton" disabled> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class name was shared between two demos, it was annoying when in the dev tools to have duplication of styles.
cursor: not-allowed; | ||
cursor: default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall MUI policy on disabled cursors.
@@ -58,23 +58,21 @@ const Button = styled(BaseButton)( | |||
&:active { | |||
background-color: ${blue[700]}; | |||
box-shadow: none; | |||
transform: scale(0.99); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It adds depth to the click interaction 🤤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @zanivan to experiment/polish/standardize!
docs/data/base/components/button/UnstyledButtonIntroduction/system/index.js
Outdated
Show resolved
Hide resolved
<Button href={'https://mui.com/'}>Standard link</Button> | ||
<Link href={'https://mui.com/'}> | ||
<Button>Next link</Button> | ||
</Link> | ||
<Button href="https://mui.com/">Standard link</Button> | ||
<Button href="https://mui.com/" slots={{ root: LinkSlot }}> | ||
Next.js link | ||
</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Next link" felt confusing, "Next.js link" instead.
const LinkSlot = prepareForSlot(Link); | ||
|
||
export default function UnstyledLinkButton() { | ||
return ( | ||
<Stack spacing={2} direction="row"> | ||
<Button href={'https://mui.com/'}>Standard link</Button> | ||
<Link href={'https://mui.com/'}> | ||
<Button>Next link</Button> | ||
</Link> | ||
<Button href="https://mui.com/">Standard link</Button> | ||
<Button href="https://mui.com/" slots={{ root: LinkSlot }}> | ||
Next.js link | ||
</Button> | ||
</Stack> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the latest recommending API for Next.js
The following demo illustrates how to use the Button as a link, whether using the Base UI Button itself for the `href`, or with the [Next.js Link component](https://nextjs.org/learn/basics/navigate-between-pages/link-component): | ||
The following demo illustrates how to use the Button as a link, whether using the Base UI Button itself for the `href`, or with the [Next.js Link component](https://nextjs.org/docs/pages/api-reference/components/link): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same URL as the other references to Next.js's Link we have in the docs.
My goal here is to fix the redirection that "https://nextjs.org/learn/basics/navigate-between-pages/link-component" is. You can find this in https://app.ahrefs.com/site-audit/3524616/91/data-explorer?columns=pageRating%2Curl%2Ctraffic%2ChttpCode%2CcontentType%2Cdepth%2Credirect%2CincomingAllLinks&filterId=b3b75b6257a370fcf9f1f73befb5deb5&issueId=c64d8847-d0f4-11e7-8ed1-001e67ed4656&sorting=-pageRating. Note that we are working to move out of Google Groups to a system that allows a lot more control. This will allow me to delegate the SEO crawl ownership. IMHO these are high priorities. I have always made everything come after. The automation is described in https://www.notion.so/mui-org/Scale-collaborative-inbox-in-Google-Groups-f6f3d7a8a6bd4c24be1fc8b13a8f33f6?pvs=4 the weekly ahrefs crawls for MUI Core will land on the replacement of [email protected] and assigned to whoever wants to own this duty. --- Notes on things that I can see while I was fixing the URL reference: 1. Updating the CSS and Tailwind CSS should be automated, I think it's a waste of time to manually maintain this. It sounds much faster to automate it. So I think that automate this is a LOT more important than adding more Tailwind CSS demos. For example, here, I fixed a border inconsistency issue. 2. We can't use :disabled. Developers might render a div, .Mui-disabled seems to be the solution. 3. I noticed 2. because the disable button was flashing. And it's not like it flashes because it's a SSR hydration. It consistently flashes on each new mount. I don't think this is possible for production, the DOM output needs to be correct from the first sync. I think we must change the API. 4. The duplication of the introduction demo is not great. I think that fixing this duplication is a LOT more important than improving the demos of the Base UI docs. The solution can be to rely on demo tabs. This will help to have higher quality demos. Right now, I think that it's completly demotivating for the community (e.g. how I felt when fixing this) to try to fix them. 5. There is a strange `useIsDarkMode` toggle in the Tailwind CSS demo. This one doesn't make sense to me. I don't see why we are not relying on the default configuration for dark mode, have it customized for the docs and configured for CodeSandbox/StackBlitz exports. 6. The code export for CodeSandbox/StackBlitz doesn't work with Tailwind CSS. 7. I find it weird to have buttonClasses.active. It doesn't seem to be superior to :active in any way. An API to remove to keep things simple? I noticed this while I added a scale(0.99) to match the box shadow removal depth effect. I find it so satisfiying 🤤, so good. 8. We still have a good number of `legacyBehavior` prop use for Next.js, even ones that leaks to developers public API. I think this should be a priority to fix, we are laggying behind.
552a74c
to
d09691f
Compare
If I understand it correctly, it should be fixed in #38943 by @mj12albert
The |
@michaldudak Ah perfect, we have an open issue for this: #38943
Oh, I see, I can reproduce this problem in https://codesandbox.io/s/fervent-snyder-vchc6f?file=/src/Demo.tsx. In https://mui.com/material-ui/react-button/#basic-button we are not supporting this case, we managed to get away from it because the |
{{"demo": "UnstyledButtonsSimple"}} | ||
{{"demo": "UnstyledButtonsSimple.js"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's too early, I'm removing these Tailwind CSS and CSS modules extra demos. I think that we only have the bandwidth for one demo per component. We are going to die on the hill if we try to convert everything without automation.
<button class="BaseButton-root"> | ||
<button class="MuiButton-root"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated
My goal here is to fix the "https://nextjs.org/learn/basics/navigate-between-pages/link-component" redirection:
https://app.ahrefs.com/site-audit/3524616/91/data-explorer?columns=pageRating%2Curl%2Ctraffic%2ChttpCode%2CcontentType%2Cdepth%2Credirect%2CincomingAllLinks&filterId=b3b75b6257a370fcf9f1f73befb5deb5&issueId=c64d8847-d0f4-11e7-8ed1-001e67ed4656&sorting=-pageRating
Note that we are working to move out of Google Groups to a system that allows a lot more control. This will allow us to delegate the SEO crawl ownership. I have handled them as high priorities so far. The automation is described in https://www.notion.so/mui-org/Scale-collaborative-inbox-in-Google-Groups-f6f3d7a8a6bd4c24be1fc8b13a8f33f6?pvs=4 the weekly ahrefs crawls for MUI Core will land on the replacement of [email protected] and assigned to whoever wants to own this duty.
But I went a bit outside of the scope of the initial problem 😇
Notes on things that I can see on Base UI documentation, stuff that surfaces quickly while I was fixing the URL reference:
:disabled
as a CSS selector. Developers might render a div,.Mui-disabled
seems to be the solution.disabled
prop not being passed across Next.js SSR #38943. I noticed 2. because the disable button flashes on each mountScreen.Recording.2023-11-12.at.06.48.56.mov
https://mui.com/base-ui/react-button/#introduction
@michaldudak I'm afraid of cases where a form may be submitted because disabled isn't set in the DOM yet while that form shouldn't be submittable.
useIsDarkMode
toggle in the Tailwind CSS demo. I don't understand why it's here. I would expect that we rely on the default configuration for dark mode, have it customized for the docs, and be configured for CodeSandbox/StackBlitz exports.I find it weird to havebuttonClasses.active
. It doesn't seem to be superior to:active
. An API to remove to keep things simple? I noticed this while I added a scale(0.99) to match the box shadow removal depth effect.legacyBehavior
props used for Next.js, even ones that leak to developers' public API. I think this should be a priority to fix since it's about keeping up with the latest dependencies, otherwise, it signals to the developers that we are lagging. [docs] Update docs related to using Next.js's Link with Material UI #38092#39954