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

[joy-ui] Add Snackbar component #38801

Merged
merged 68 commits into from
Oct 24, 2023
Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Sep 4, 2023

@ZeeshanTamboli ZeeshanTamboli added new feature New feature or request component: snackbar This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Sep 4, 2023
@mui-bot
Copy link

mui-bot commented Sep 4, 2023

Netlify deploy preview

@mui/joy: parsed: +1.93% , gzip: +1.75%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 42f0ebb

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 10, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 12, 2023
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.

👍 Thanks @ZeeshanTamboli

@mnajdova
Copy link
Member

The component looks great. Few points for improving the demos:

  • The solid variant on the playground does not look good without setting the invariant color prop. Should we have default true for it? How do we teach people that this is the recommendation? cc @siriwatknp

    Screenshot 2023-10-20 at 13 11 33
  • The colors demo is too distracting. I would show the default variant only, we have the playground for the combination of the props. Once we have the stacking as a feature we can show them all stacked.

  • On the hide duration, I would decrease the counter by a bigger step

@siriwatknp
Copy link
Member

The component looks great. Few points for improving the demos:

  • The solid variant on the playground does not look good without setting the invariant color prop. Should we have default true for it? How do we teach people that this is the recommendation? cc @siriwatknp
    Screenshot 2023-10-20 at 13 11 33
  • The colors demo is too distracting. I would show the default variant only, we have the playground for the combination of the props. Once we have the stacking as a feature we can show them all stacked.
  • On the hide duration, I would decrease the counter by a bigger step

I would leave the first point as is because applying color inversion by default could be too magical for some people and it is not efficient with zero-runtime in the future. We should let developers compose this feature by themselves.

Agree with the rest!

@zanivan
Copy link
Contributor

zanivan commented Oct 20, 2023

I would leave the first point as is because applying color inversion by default could be too magical for some people and it is not efficient with zero-runtime in the future. We should let developers compose this feature by themselves.

Then, IMO it's important to make this explicit in the docs—maybe on the inverted colors' demo. Otherwise, it'll look off, and might be mistaken for a bug.

@mnajdova
Copy link
Member

Then, IMO it's important to make this explicit in the docs—maybe on the inverted colors' demo. Otherwise, it'll look off, and might be mistaken for a bug.

+1, if we don't plan to fix this (which I disagree with), we should remove this solid variant from the playground. It doesn't make sense to show a broken demo regardless of what are the technical limitations.

@siriwatknp
Copy link
Member

@mnajdova @zanivan I removed the inverted colors from the playground and updated the content to look good in all variants. Also, make bottom-right as the default position of the snackbar.

Ready for the last review.

@ZeeshanTamboli
Copy link
Member Author

I made the open prop required after noticing the upvote in #38801 (comment).


On the hide duration, I would decrease the counter by a bigger step

Agreed. Decremented the duration counter by 100ms instead of 16ms.

Ready for review!

@siriwatknp siriwatknp added this to the Joy UI: Stable release milestone Oct 23, 2023
Copy link
Contributor

@zanivan zanivan left a comment

Choose a reason for hiding this comment

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

Design-wise looks great! 🚀

@siriwatknp siriwatknp merged commit e1d5cb8 into mui:master Oct 24, 2023
22 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the joy-ui-snackbar branch October 24, 2023 16:47
@mnajdova
Copy link
Member

Great contributon @ZeeshanTamboli! Thank you 🙏

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 10, 2023

It's great to see one more component in Joy UI.

One thing I noticed from a design perspective that feels off is the distance to the edge of the screen. Feedback added to https://www.notion.so/mui-org/Olivier-design-review-on-Joy-Design-3ada9a7bcfa44b9fab1fe5032dfb33bb?pvs=4#559b0978badf4daba4af9aa712bb1fea.

(Eventually, I think we should move all of this document to GitHub issues, there might be ~100 design GitHub issues to create, if we want to go very granular 😁)

@zanivan
Copy link
Contributor

zanivan commented Nov 10, 2023

One thing I noticed from a design perspective that feels off is the distance to the edge of the screen

Agreed! I'll take a look on it!

(Eventually, I think we should move all of this document to GitHub issues, there might be ~100 design GitHub issues to create, if we want to go very granular 😁)

I'll revisit the file and double check those items. We've already dealt with or thrown away many of those, but there's definitely a lot of feedback that we could turn into issues! For instance, this feedback about tabs fits perfectly with what we're trying to do on #39363 😁

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! new feature New feature or request package: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

[Joy] Add the Snackbar component
8 participants