material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[material-ui][Badge] Fix stale values being used when badge is invisible

Open DiegoAndai opened this issue 1 year ago • 2 comments

Fixes: https://github.com/mui/material-ui/issues/43081

To see the error:

  • Go to the before/after demo
  • Inspect the .MuiBadge-badge element
  • Change the value back and forth between 8 and 0
  • Observe the .MuiBadge-badge element's value

Before: https://stackblitz.com/edit/react-qkjzbj?file=Demo.tsx After: Waiting for CI build

I think the use of usePreviousProps is incorrect, as badgeContent, max and displayValue shouldn't be coupled with the invisible value. They should always be updated regardless. It was originally added for another use case: https://github.com/mnajdova/material-ui/commit/7572c905d57a5e44630dc1980ea1395424dd0947#diff-26877d58ee4ebaa271700aae39d84f0ce250e8d01c677024bbf0d53f46159b1b, so it might've stuck around inadvertently.

I fixed in both mui-material and mui-base for consistency.

DiegoAndai avatar Jul 26 '24 16:07 DiegoAndai

Netlify deploy preview

https://deploy-preview-43083--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against 6a98b91d2549b1480704237608105f7489cc59bd

mui-bot avatar Jul 26 '24 16:07 mui-bot

I think the use of usePreviousProps is incorrect, as badgeContent, max and displayValue shouldn't be coupled with the invisible value. They should always be updated regardless. It was originally added for another use case: https://github.com/mnajdova/material-ui/commit/7572c905d57a5e44630dc1980ea1395424dd0947#diff-26877d58ee4ebaa271700aae39d84f0ce250e8d01c677024bbf0d53f46159b1b, so it might've stuck around inadvertently.

I would recommend when going trough history to search for the PR that did the fix, as it is usually how you can understand the motivation for the code change, e.g. https://github.com/mui/material-ui/pull/21557 has a description & explanation. Also, changing a test should be an indication that something will be broken/behavior will change.

mnajdova avatar Jul 29 '24 07:07 mnajdova