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

[Badge] Fix transition flicker #21557

Merged
merged 6 commits into from Jun 25, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jun 24, 2020

This PR is fixing the flickering of the badge components, when transitioning to invisible state.

Previously:
bug-badge

(You can see the middle transition - the 0 being applied while it is disappearing)

Now:
fix-badge

The provided solution is based on the prev properties on the Badge component (only the props used in the classes + the content are being considered.

As an alternative I tried using the SwitchTransition component from react-transition-group, but the flickering was still there...

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 24, 2020

@material-ui/core: parsed: +0.07% , gzip: +0.12%

Details of bundle changes

Generated by 🚫 dangerJS against 8fcbd11

@mnajdova mnajdova added bug 🐛 Something doesn't work component: badge This is the name of the generic UI component, not the React module! labels Jun 24, 2020
@mnajdova mnajdova marked this pull request as ready for review June 24, 2020 15:28
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.

I can confirm it work, It doesn't solve 100% of the problem, but it sounds pragmatic.

There is something I have been wondering about the Badge component for some time, is whether or not we should break it down. It seems that the positioning and animation logic could be isolated from the display. For instance: https://getbootstrap.com/docs/4.5/components/badge/#contextual-variations.

packages/material-ui/src/Badge/Badge.js Outdated Show resolved Hide resolved
packages/material-ui/src/Badge/Badge.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Badge/Badge.js Outdated Show resolved Hide resolved
@mbrookes
Copy link
Member

@oliviertassinari

It doesn't solve 100% of the problem

Out of curiosity, what part doesn't it solve?

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mnajdova
Copy link
Member Author

There is something I have been wondering about the Badge component for some time, is whether or not we should break it down. It seems that the positioning and animation logic could be isolated from the display. For instance: https://getbootstrap.com/docs/4.5/components/badge/#contextual-variations.

On this I had a similar thought. I think there is too much opinion baked inside the Badge. I can easily as a client add the disappearing as a logic on top of it (or maybe we should have customization example, or some transition component that can handle this) In my opinion the badge should be just a small dot that contain some info optionally and can be positioned around an element... Not sure if it used internally in other components currently, but even on client's side, there shouldn't be any penalty for functionality that may not be used always... Anyway, I can see the benefit of clients not needing to handle this on their side, so it is about the less opinionated/more opinionated opposition :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 24, 2020

Out of curiosity, what part doesn't it solve?

@mbrookes From what I understand, if the component commit twice, we will notice the 0. But IMHO, it's good enough in the current tradeoff :).

I think there is too much opinion baked inside the Badge

Ok cool, there is a Trello item that is dedicated to this problem. It's not directly related to this problem we try to solve, we can discuss it later on.

mnajdova and others added 2 commits June 24, 2020 20:26
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari oliviertassinari merged commit 57322a9 into mui:next Jun 25, 2020
@oliviertassinari
Copy link
Member

@mnajdova Well done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: badge This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants