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 Snackbar page #39298

Merged
merged 17 commits into from
Jan 15, 2024

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented Oct 3, 2023

Was checking out the docs-feedback channel, and someone recently posted that they spent a lot of time searching how to center a snackbar. Then, I visited the page and found that there were some formatting/writing improvements that might facilitate people finding information better the next time.

My overall train of thought for this was to remove most of the "how-to-use" instructions from a UX standpoint (parting from the assumption that the Material Design docs itself are the place for that), and focus on the component's capacities. Also, tried to follow the structure we've been applying across the Base UI and Joy UI docs and simplified some of the demos!

👉 https://deploy-preview-39298--material-ui.netlify.app/material-ui/react-snackbar/

@danilo-leal danilo-leal added docs Improvements or additions to the documentation component: snackbar This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Oct 3, 2023
@danilo-leal danilo-leal self-assigned this Oct 3, 2023
@mui-bot
Copy link

mui-bot commented Oct 3, 2023

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against bc1c817

Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Thanks for working on it!

I have only reviewed the text and not the code changes.

docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
@samuelsycamore
Copy link
Contributor

samuelsycamore commented Oct 24, 2023

I was just starting to dig into this doc for another pass when I noticed the color contrast on these demos! 😬 I think the Introduction, Content, and Consecutive demos could use some love.
Screenshot 2023-10-24 at 8 05 27 AM

docs/data/material/components/snackbars/SimpleSnackbar.tsx Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
@danilo-leal danilo-leal changed the title [docs][material-ui] Revise the Snackbar page [material-ui][docs] Revise the Snackbar page Dec 21, 2023
@danilo-leal
Copy link
Contributor Author

danilo-leal commented Dec 22, 2023

@samuelsycamore just added the section about the autoHideDuration prop we discussed in a comment above! Mind taking a second look? I think the button's design issue there, which you mentioned is either a problem with the component itself or in the docs, as the Snackbar flips from a dark-colored background to a white one when you switch modes. 🤔

@mbrookes
Copy link
Member

mbrookes commented Dec 27, 2023

Off topic, but what's up with this?

image

@danilo-leal
Copy link
Contributor Author

☝️ That's been removed/corrected in this PR! 😬

Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

I'm pretty sure this is the cleanest component demo page that exists in the Material UI docs now! 🥳 Left one final comment to think about, but shouldn't be a blocker for this PR.

docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
@danilo-leal
Copy link
Contributor Author

@mbrookes can you double-check this one when possible? Feels like it's ready to go! :)

Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Minor tweaks.

@mbrookes
Copy link
Member

In the Transitions demo, if you don't close an open Snackbar first, then when clicking a different button the transition doesn't render correctly. Since it might overcomplicate the demo to fix this, should we add a note to the text to explain that this? Or would adding a short autoHide duration be enough?

@mbrookes
Copy link
Member

mbrookes commented Jan 11, 2024

Regarding the change of case from snackbar to Snackbar, the original intention across the docs was to distinguish between, for example, a button in the general sense (lower case) and an instance of a Material UI Button component (capitalized).

I suspect that distinction has long been lost, and it was sometimes ambiguous anyway, so the only case I would argue for it on this page is "Material Design recommends positioning Snackbars" - it doesn't recommend positioning React Snackbar components specifically. It's not a hill I'll die on if anyone disagrees.

@mbrookes
Copy link
Member

I've submitted an approving review because for some reason merging was blocked even though there was already one approving review which is all the repo rules require...

@mbrookes
Copy link
Member

One last comment - the Consecutive Snackbars demo immediately dismisses the open Snackbar the moment another one is added, whereas it feels like it should queue them.

@danilo-leal
Copy link
Contributor Author

@DiegoAndai tried pulling off Matt's suggestion for the consecutive Snackbar demo myself, but just couldn't make two instances appear consecutively with the click on one button only — when possible, can you help out with that one? :)

@danilo-leal
Copy link
Contributor Author

I'll merge this one in its current state so we get the overall improvements we've added here, and then any additional changes can come in further PRs iteratively — including the one mentioned above (i.e., consecutive snackbars demo). 😃

@danilo-leal danilo-leal merged commit 141dcfe into mui:master Jan 15, 2024
19 checks passed
@danilo-leal danilo-leal deleted the revise-material-snackbar-page branch January 15, 2024 19:45
@DiegoAndai
Copy link
Member

couldn't make two instances appear consecutively with the click on one button only — when possible, can you help out with that one? :)

Hey @danilo-leal! So, if I understood correctly, the idea is to queue the snackbars instead of immediately showing them? It seems to me that it may be confusing, like the snackbars are "lagging." I think the timing of snackbars is up to the user's app workflow rather than on our side. The demo seems straightforward to show the transition, so users can take it and integrate it with their app's logic.

What do you think?

CC: @mbrookes

@mbrookes
Copy link
Member

I know this is merged already, but I had an unsubmitted comment:

Sam previously mentioned the contrast ratio of the Content demo. It wasn't always that bad, but was never great. Here's v4:
image
Any chance you could fix that before we wrap this up?

One for a follow-up...

@mbrookes
Copy link
Member

I think the timing of snackbars is up to the user's app workflow rather than on our side.

Which is what our demos demonstrate - code for using the components (with sensible UX and accessibility). Sorry if I'm stating the obvious here, or missing your point, but you seem to be arguing for the opposite.

@DiegoAndai
Copy link
Member

Which is what our demos demonstrate - code for using the components (with sensible UX and accessibility). Sorry if I'm stating the obvious here, or missing your point, but you seem to be arguing for the opposite.

I argue that the Consecutive Snackbars demo aims to show how to transition between consecutive snackbars without stacking. For this reason, showing that transition right away is better, as that's the main point of the demo.

Queueing seems to be a separate use case. Sure, you can use both, but keeping the demos "atomic" would be better for users. It's easier to merge two demos than to pick one apart for what you need.

@mbrookes
Copy link
Member

I just reread your original comment in that context, and I take your point. And having played with queuing I see what you mean about feeling laggy, as nothing appears to happen when you click the button. Let's leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: snackbar 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
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants