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-next][ProgressIndicator] Apply MD3 style to LinearProgress #39807

Merged
merged 23 commits into from
Nov 28, 2023

Conversation

lhilgert9
Copy link
Contributor

@lhilgert9 lhilgert9 commented Nov 8, 2023

LinearProgress issue: #39778
Material You umbrella issue: #29345
Preview: https://deploy-preview-39807--material-ui.netlify.app/material-ui/react-progress/#material-you-version

Changes

  • Change color props
  • Apply MD3 style
  • Add playground to docs

@mui-bot
Copy link

mui-bot commented Nov 8, 2023

Netlify deploy preview

@mui/material-next: parsed: +1.71% , gzip: +1.31%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 89d6077

@lhilgert9
Copy link
Contributor Author

Regarding the animations:
Other frameworks have completely restructured the LinearProgress for MD3 and added new elements to maintain the animation. The question is whether we should do the same or find another way to achieve the same visual result.

Example:

Does anyone have any ideas on this?

@lhilgert9 lhilgert9 marked this pull request as ready for review November 8, 2023 20:15
@mnajdova mnajdova added component: progress This is the name of the generic UI component, not the React module! v6.x design: material you labels Nov 9, 2023
@DiegoAndai DiegoAndai self-requested a review November 20, 2023 13:36
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @lhilgert9, thanks for working on these! Sorry for the delay in the review, I was OOO.

Left some questions. I also think we should add the playground in this PR, that way we can easily see the updated component.

@lhilgert9
Copy link
Contributor Author

I also think we should add the playground in this PR, that way we can easily see the updated component.

@DiegoAndai You mean the playground in the docs?

@DiegoAndai
Copy link
Member

@DiegoAndai You mean the playground in the docs?

Yes 👍🏼

@lhilgert9
Copy link
Contributor Author

lhilgert9 commented Nov 20, 2023

@DiegoAndai Playground is implemented👍🏼
Link is in the initial comment.

@DiegoAndai
Copy link
Member

I'm bringing @zanivan into this as a design reviewer 😊

The specs default for the four-color option doesn't look good. I suggest choosing a better default on our side. If users want to use the specs, they can override the variables. What do you think @zanivan @lhilgert9?

@lhilgert9
Copy link
Contributor Author

@DiegoAndai I agree with you that these don't look nice. Here we should also consider adding the additional inner-bar elements as already suggested (#39807 (comment))

@zanivan
Copy link
Contributor

zanivan commented Nov 22, 2023

The specs default for the four-color option doesn't look good. I suggest choosing a better default on our side. If users want to use the specs, they can override the variables. What do you think @zanivan @lhilgert9?

I was digging further to understand why the material web looks different from our version, even though using the same colors: primary to primary-container, and then tertiary to tertiary-container.

What I think that might have happened to their version look darker is that instead of primary-container and tertiary-container, it could actually be on-primary-container and on-tertiary-container—and I believe we should use these too, anyway.

@lhilgert9
Copy link
Contributor Author

@zanivan I tried it out and yes, it looks much better. What do you think about adding the inner-bar elements like in material-web to get the animation right, as I'm not really happy with the current one?

@zanivan
Copy link
Contributor

zanivan commented Nov 23, 2023

@zanivan I tried it out and yes, it looks much better.

Good one! It looks much better indeed 🙌

What do you think about adding the inner-bar elements like in material-web to get the animation right, as I'm not really happy with the current one?

This will likely be better answered by @DiegoAndai, but IMO, if the structure with the inner-bar is better aligned with the new material design structure, maybe we should go for it? I mean, if it's not a huge breaking change.
The current one is not that bad, though!

@lhilgert9
Copy link
Contributor Author

@DiegoAndai I would suggest that we merge this PR after review. Then we could open a new PR/draft and try the idea with the inner-bar elements in this one. I would leave this PR as it is, as @zanivan also said that the design is fine for now.

@DiegoAndai
Copy link
Member

@DiegoAndai I would suggest that we merge this PR after review. Then we could open a new PR/draft and try the idea with the inner-bar elements in this one. I would leave this PR as it is, as @zanivan also said that the design is fine for now.

I agree ✅

Things missing to merge this PR are:

What do you think about adding the inner-bar elements like in material-web to get the animation right, as I'm not really happy with the current one?

For this refactor:

  • What breaking changes would you expect we would introduce?
  • What's the difference in the animation we're looking to achieve?

Thanks @zanivan for the review and help with the four-color design 🎉

@lhilgert9
Copy link
Contributor Author

@DiegoAndai

Things missing to merge this PR are:

I have now fixed these things. So this PR should now ready to merge.

For this refactor:

  • What breaking changes would you expect we would introduce?
  • What's the difference in the animation we're looking to achieve?

I took another look at it and yes, it actually looks good the way it is right now. Let's just leave it as it is, then we won't have any more braking changes.

@DiegoAndai DiegoAndai merged commit 419692d into mui:master Nov 28, 2023
22 checks passed
@DiegoAndai
Copy link
Member

Great job @lhilgert9!

@lhilgert9 lhilgert9 deleted the apply-md3-style-to-linear-progress branch November 29, 2023 07:11
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 30, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 1, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 1, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
@DiegoAndai DiegoAndai added v7.x and removed v6.x labels Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: progress This is the name of the generic UI component, not the React module! design: material you v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants