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] Improve demos #18981

Merged
merged 3 commits into from
Jan 3, 2020
Merged

Conversation

ypresto
Copy link
Contributor

@ypresto ypresto commented Dec 25, 2019

Orignal Title: [Badge] Retain text, color and variant while disappearing

Currently, Badge will apply all appearance immediately even while disappearing transition.

It perhaps causes flicker in some cases like:

  • Show red badge with counter if the user has an urgent notification.
  • Otherwise show blue badge with counter.
  • <Badge color={hasUrgent ? 'error' : 'secondary'} badgeContent={count}>
  • Then it unwantedly shows disappearing blue 0 badge after the user opens notification.

Simulating such situation with demo:

docs/src/pages/components/badges/BadgeVisibility.js

-        <Badge color="secondary" badgeContent={4} invisible={invisible} className={classes.margin}>
+        <Badge color={invisible ? 'primary' : 'secondary'} badgeContent={invisible ? 0 : 4} className={classes.margin}>

image
image
image

This PR fixes that flicker: text, color and variant will be applied at next appearing.

@ypresto ypresto force-pushed the badge-leave-transition-fix branch from 2c8bf3e to 7a8ed11 Compare December 25, 2019 19:19
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 25, 2019

No bundle size changes comparing ee46512...fa9e6b2

Generated by 🚫 dangerJS against fa9e6b2

@ypresto ypresto force-pushed the badge-leave-transition-fix branch from 7a8ed11 to 8a7b957 Compare December 25, 2019 19:40
@ypresto ypresto changed the title [Badge] Retain text, color and variant while hide transition [Badge] Retain text, color and variant while disappearing Dec 25, 2019
@ypresto ypresto force-pushed the badge-leave-transition-fix branch from 8a7b957 to c6e2422 Compare December 25, 2019 19:47
@ypresto
Copy link
Contributor Author

ypresto commented Dec 26, 2019

Browser test was failed with timeout. Could I retry it?

@mbrookes mbrookes added the component: badge This is the name of the generic UI component, not the React module! label Dec 28, 2019
@oliviertassinari
Copy link
Member

I don't understand, what problem do the changes solve?

@ypresto
Copy link
Contributor Author

ypresto commented Dec 31, 2019

Changing the appearance of a badge while disappearing animation (like badgeContent={1} to {0}, or changing color prop) causes a flickering experience. So this PR keeps the appearance while the animation. It is an enhancement, not a bug.

@oliviertassinari
Copy link
Member

Should it be handled in userland? What's the use case for changing the appearance during an animation?

@ypresto
Copy link
Contributor Author

ypresto commented Dec 31, 2019

I think most of the users write badgeContent={count} and set count to 0 to hide a badge, and it effectively changes appearance (content text) at the start of the animation.

Color must be less frequently changed, but writing something like <Badge color={hasUrgent ? 'error' : 'secondary'}> and resetting hasUrgent flag at hiding also causes flicker.

(I checked Google Analytics (which uses different colors for same badge, but maybe different from Material), and it does not animate at disappearing.)

@oliviertassinari oliviertassinari added the new feature New feature or request label Dec 31, 2019
@oliviertassinari oliviertassinari force-pushed the badge-leave-transition-fix branch 2 times, most recently from 8d0a5a0 to d2db5f7 Compare December 31, 2019 15:14
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 31, 2019

@ypresto Ok thanks for sharing the use case. I don't think that we should care about the color/variant changes. Regarding the 0 -> 1 jump. I have updated the demo to reproduce it and updated the implementation with a simple patch. If this works for you, I think that it would be great to add a test case (with testing-library, not enzyme).

@oliviertassinari oliviertassinari changed the title [Badge] Retain text, color and variant while disappearing [Badge] Improve animation Dec 31, 2019
@oliviertassinari oliviertassinari force-pushed the badge-leave-transition-fix branch from d2db5f7 to 356296b Compare December 31, 2019 15:20
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation and removed new feature New feature or request labels Jan 3, 2020
@oliviertassinari oliviertassinari changed the title [Badge] Improve animation [Badge] Improve demos Jan 3, 2020
@oliviertassinari
Copy link
Member

@ypresto I have sent one commit to focus on the demo only.

Regarding the animation problem. I can't wrap my head around a viable solution. Any idea?

@oliviertassinari oliviertassinari force-pushed the badge-leave-transition-fix branch from 7a8de22 to fa9e6b2 Compare January 3, 2020 10:52
@oliviertassinari oliviertassinari merged commit d9ced3a into mui:master Jan 3, 2020
m4theushw pushed a commit to m4theushw/material-ui that referenced this pull request Jan 6, 2020
@ypresto
Copy link
Contributor Author

ypresto commented Jan 14, 2020

I was in new year holidays 🙏

leave it to userland until we have enough time to deticate to it.

I agree with you. Might better to have demo, but perhaps someone want to fix it can find this PR. 😺

@oliviertassinari
Copy link
Member

@ypresto I hope you had great holidays! :D.
I'm still wondering what would be a great solution for this problem 🤔. It feels like it would need a new abstraction for the transition to correctly solve it. It's also related to the issues with have with react-transition-group (strict mode, bundle size, no hooks).

@ypresto
Copy link
Contributor Author

ypresto commented Jan 19, 2020

react-transition-group (strict mode, bundle size, no hooks).

Oh, it's somewhat legacy and not retrieving sufficient maintenance? reactjs/react-transition-group#429 🤔

But its <SwitchTransition> looks good solution for this! (I have just known it).

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 16, 2020

It seems that the documentation has this exact same issue with the notification. Notice the double state change 2 -> 0 -> invisible.

Feb-16-2020 13-57-02

cc @mbrookes

@mbrookes
Copy link
Member

Ah, it was hidden when the popper content covered the icon, so hadn't noticed.

What about this diff?:

index cf1fcef48..1ddcfe62b 100644
--- a/packages/material-ui/src/Badge/Badge.js
+++ b/packages/material-ui/src/Badge/Badge.js
@@ -171,6 +171,8 @@ const Badge = React.forwardRef(function Badge(props, ref) {
     ...other
   } = props;
 
+  const [lastContent] = React.useState(badgeContent, []);
+
   let invisible = invisibleProp;
 
   if (
@@ -184,6 +186,7 @@ const Badge = React.forwardRef(function Badge(props, ref) {
 
   if (variant !== 'dot') {
     displayValue = badgeContent > max ? `${max}+` : badgeContent;
+    displayValue = badgeContent === 0 && !showZero ? lastContent : badgeContent;
   }
 
   return (

@oliviertassinari
Copy link
Member

@mbrookes Does it work with consecutive updates?

@mbrookes
Copy link
Member

It works with the docs +/- demo, but as soon as I posted it, I realised that for the notifications icon, it's still showing showing 0 before transitioning out. 🤷🏽‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: badge This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants