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

[material-ui][docs] Revise the Button page #40446

Closed
wants to merge 20 commits into from
Closed

[material-ui][docs] Revise the Button page #40446

wants to merge 20 commits into from

Conversation

samuelsycamore
Copy link
Contributor

@samuelsycamore samuelsycamore commented Jan 5, 2024

Part of #35158

  • thoroughly restructured the doc to match the current format
  • rewrote (and removed) sections that plagiarized the Material Design specs
  • added demos for previously undocumented props

https://deploy-preview-40446--material-ui.netlify.app/material-ui/react-button

@samuelsycamore samuelsycamore added docs Improvements or additions to the documentation component: button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Jan 5, 2024
@samuelsycamore samuelsycamore self-assigned this Jan 5, 2024
@mui-bot
Copy link

mui-bot commented Jan 5, 2024

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 0725e8a

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Woof, this is much, much better! Great work 🎉

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 11, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 12, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 13, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 23, 2024
@samuelsycamore
Copy link
Contributor Author

Alright, I've done some heavy revising since the last review but I think this is ready to go now!

@@ -1,10 +1,14 @@
import * as React from 'react';
import Stack from '@mui/material/Stack';
Copy link
Member

Choose a reason for hiding this comment

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

We can't do this yet, but this could make a lot of sense for v6 #40594.

Suggested change
import Stack from '@mui/material/Stack';
import Stack from '@mui/system/Stack';

docs/data/material/components/buttons/buttons.md Outdated Show resolved Hide resolved
Comment on lines +15 to +17
onClick={() => {
alert('Canceled.');
}}
Copy link
Member

Choose a reason for hiding this comment

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

To consider if we should remove the onClick to get the "inline preview"


You can remove the elevation with the `disableElevation` prop.
```jsx
<Button href="/home">Home</Button>
Copy link
Member

@oliviertassinari oliviertassinari Jan 30, 2024

Choose a reason for hiding this comment

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

#suggestion I would enjoy a live demo here as a developer.

Comment on lines +7 to +14
<Box
sx={{
width: '300px',
border: '1px solid gray',
borderRadius: '5px',
padding: '5px',
}}
>
Copy link
Member

Choose a reason for hiding this comment

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

To get the inline preview?

Suggested change
<Box
sx={{
width: '300px',
border: '1px solid gray',
borderRadius: '5px',
padding: '5px',
}}
>
<div>
<Box
sx={{
width: '300px',
border: '1px solid gray',
padding: '5px',
}}
>

Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to suggest whether we need the 300px width limit there... I think we can make this one span the full demo container width, given it's illustrating the fullWidth prop.

Copy link
Member

@oliviertassinari oliviertassinari Feb 4, 2024

Choose a reason for hiding this comment

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

I was about to suggest whether we need the 300px width limit there... I think we can make this one span the full demo container width, given it's illustrating the fullWidth prop.

A direction that makes sense to me:

  • move the header from h4 to h3. Even if it's technical under Sizes, IMHO it can also been seen as not, and it would help with anchor linking.
  • rename the header to Full width, since it's what this demo is about
  • Increase 300px to some a bit more, up to the limit where it starts to feel poor UX.
  • use "bg": true

Screenshot 2024-02-04 at 22 47 05

Edit: actually, more padding y than padding x could make it 🤌

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the background, though; it looks cleaner without it (plus all the other demos don't use it).

docs/data/material/components/buttons/buttons.md Outdated Show resolved Hide resolved
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.

@oliviertassinari oliviertassinari self-requested a review January 30, 2024 17:19
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

This looks super good — left just tiny remarks; excited to see it getting shipped!


{{"demo": "TextButtons.js"}}
## Basics
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Basics
## Basics
### Import

Should we add it? We did it on the Snackbar page, but I guess we never formalized it as part of the template!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah interesting, I must have glossed over it on that page (and it may have been omitted in the other pages we've updated recently). I don't have a strong opinion about it one way or the other, as long as we apply it consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, as I'm thinking about it more, I might argue against including it. The main reason being that we sometimes include imports in other sections (when discussing complementary components) and it could be cluttered/redundant if we added it for each one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uhm, that makes sense, yeah! Well, I have no strong preferences either, so I'm happy to go without it, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tangential, but I was testing out the search experience for "(component) import" and this is what it looks like for Snackbar:

Screenshot 2024-02-05 at 10 00 14 AM

That's a lot of pages and headers that look identical! 😅 How do I know which one to click on? We should give some more thought to the layout of the search results and how it relates to repeated headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, these first two Base UI results, for example, are from the Components API and Hooks API tabs, respectively, which makes me think we should probably include them somehow in the results (so you know what that's about).

Copy link
Member

@oliviertassinari oliviertassinari Feb 11, 2024

Choose a reason for hiding this comment

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

The problem here to me is how we rank Algolia results. If I'm on a Material UI page, no way I want to see Base UI or MUI System results in between. I want first as section that is all about Material UI results, and second search area for other products.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's another problem on top of the one Sam shared. Imagine if Sam's screenshot is a search result when browsing through the Base UI docs. The results order is correct, but still, there's an opportunity to improve how we differentiate pages or sections with the same heading text.

deselected, such as adding or removing a star to an item.
:::warning
These props remove _all_ default styles from the `:focus-visible` state, which means the user won't have any visual indication that the Button is focused.
You can target the `.Mui-focusVisible` class to define your own styles for a better user experience.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can target the `.Mui-focusVisible` class to define your own styles for a better user experience.
You can target the `.Mui-focusVisible` class to define your own styles for an accessible user experience.

Thought of calling out the accessibility impact of having properly styled focus styles here instead of saying "better"; maybe that's more specific... and better? 😅

@oliviertassinari
Copy link
Member

Off-topic

The <span with background-image looks outdated now

Screenshot 2024-02-04 at 22 49 58

best to use the object-fit CSS property now, so we can use loading="lazy" on the image

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 6, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 14, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 15, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2024
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

We can merge this in the next branch next week.

@samuelsycamore samuelsycamore closed this by deleting the head repository Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants