Fix not rerendering days when inCurrentMonth changes #1994
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/q0g7dv3ql |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| @@ -77,7 +77,7 @@ export const SlideTransition: React.SFC<SlideTransitionProps> = ({ | |||
| }) => { | |||
| const classes = useStyles(); | |||
| if (reduceAnimations) { | |||
| return <div className={className}>{children}</div>; | |||
| return <div className={clsx(classes.root, className)}>{children}</div>; | |||
There was a problem hiding this comment.
Should we spread too?
| return <div className={clsx(classes.root, className)}>{children}</div>; | |
| return <div className={clsx(classes.root, className)} {...other}>{children}</div>; |
There was a problem hiding this comment.
Here props are spreading to the TransitionGroup. So no, we don’t need to spread them
There was a problem hiding this comment.
Right, because SlideTransition is private so it doesn't matter
| @@ -251,6 +251,7 @@ export const areDayPropsEqual = (prevProps: DayProps, nextProps: DayProps) => { | |||
| prevProps.showDaysOutsideCurrentMonth === nextProps.showDaysOutsideCurrentMonth && | |||
| prevProps.disableHighlightToday === nextProps.disableHighlightToday && | |||
| prevProps.className === nextProps.className && | |||
| prevProps.inCurrentMonth === nextProps.inCurrentMonth && | |||
There was a problem hiding this comment.
Didn't reproduce without reduceAnimation because transition group is mounting and unmounting the whole component on animation.
What do you think of dropping the areDayPropsEqual logic? I believe that if we drop the ButtonBase component as proposed #1674, we would make this optimization with almost no ROI.
There was a problem hiding this comment.
We could test it, but without this function everything is SO slow especially for dateRange picker.
There was a problem hiding this comment.
I'm confused. Here are my thoughts:
- What's the difference with the date range picker that explains the slowness compared to the date picker?
- We display two calendars instead of one.
- Ok, so the slowness is only an issue with desktop as we display a single calendar on mobile.
- On desktop we don't use
reduceAnimation. - You said that the logic is only working if
reduceAnimation
I come to the conclusion that it shouldn't have any impact on date range picker. Where is the error?
There was a problem hiding this comment.
No it also works for DateRangePicker preview and in month selection and keyboard control.
There was a problem hiding this comment.
keyboard control
The focus keybroad logic could be bypassing React entirely
|
I think this kind of bugs don’t need a pinning test, because you will delete this test once will see it in the future |
There was a problem hiding this comment.
I have added a note about a good opportunity to simplify the codebase with removing areDayPropsEqual in #1674
I think this kind of bugs don’t need a pinning test, because you will delete this test once will see it in the future
I agree because the probability that we will be able to drop this logic in the near future is probably 80% :)
|
I am 99% sure we will not drop this logic. Hovering through JavaScript (Date range preview) is too slow if all 36 components rerenders. |
|
Good point, I didn't account for the date range selection, for which, we might not want to bypass React for rendering it. In this case, we miss a test case at 99% ;) |
|
Especially considering that it fixes two GitHub issues. |
Closes #1993 and also closes #1756
The root of this issue was missing check for
inCurrentMonthinReact.memo. The same day with the samekeywas rendered in the new month, so react basically didn't change it only moved the node from one place to another one.Didn't reproduce without
reduceAnimationbecause transition group is mounting and unmounting the whole component on animation.