feat: month animation#2684
Conversation
gpbl
left a comment
There was a problem hiding this comment.
This looks so great, I love how you nailed it!
Next steps to me would be:
- Move the
useLayoutEffectinto auseAnimationhook. - Add a test for the new
useAnimation. - Improve readability with some refactoring.
| /** CSS classes used for animating months and captions. */ | ||
| export enum Animation { | ||
| /** Applied to the entering month when it is after the exiting month. */ | ||
| animation_enter_month_weeks_is_after = "animation_enter_month_weeks_is_after", |
There was a problem hiding this comment.
@gpbl FYI, I changed the name of these classes to month_weeks to follow the same patter of the month_caption and be more explicit that it's being applied to the Weeks component.
Lemme know what you think.
There was a problem hiding this comment.
I love everything but the verbosity here :) What about:
export enum Animation {
weeks_next_enter = "weeks_next_enter",
weeks_next_exit = "weeks_next_exit",
weeks_prev_enter = "weeks_prev_enter",
weeks_prev_exit = "weeks_prev_exit",
caption_next_enter = "caption_next_enter",
caption_next_exit = "caption_next_exit",
caption_prev_enter = "caption_prev_enter",
caption_prev_exit = "caption_prev_exit"
}There was a problem hiding this comment.
Fanstastic work @rodgobbi !
The work is done here and we may want to polish a bit the code before merging.
My suggestions:
useAnimation: Remove the return value and clean up the arguments.- Shorten CSS class and keyframe names to avoid cumbersome customization.
- Rename animation-related data attributes like
data-month-container→data-animated-month.
If you prefer, I can propose my suggested changes in a separate branch.
| .rdp-animation_enter_month_weeks_is_after { | ||
| animation: rdp-animation_slide_in_right_keyframes var(--rdp-animation_duration) var(--rdp-animation_timing) forwards; | ||
| } | ||
|
|
||
| .rdp-animation_exit_month_weeks_is_before { | ||
| animation: rdp-animation_slide_out_left_keyframes var(--rdp-animation_duration) var(--rdp-animation_timing) forwards; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think a shorter names will help "grouping" these classes and ease up further customization:
| .rdp-animation_enter_month_weeks_is_after { | |
| animation: rdp-animation_slide_in_right_keyframes var(--rdp-animation_duration) var(--rdp-animation_timing) forwards; | |
| } | |
| .rdp-animation_exit_month_weeks_is_before { | |
| animation: rdp-animation_slide_out_left_keyframes var(--rdp-animation_duration) var(--rdp-animation_timing) forwards; | |
| } | |
| .rdp-weeks_next_in { | |
| animation: rdp-slide_in_next var(--rdp-animation_duration) var(--rdp-animation_timing) forwards; | |
| } | |
| .rdp-weeks_prev_out { | |
| animation: rdp-slide_out_prev var(--rdp-animation_duration) var(--rdp-animation_timing) forwards; | |
| } |
There was a problem hiding this comment.
Indeed sounds better being shorter.
But a couple of points to discuss about the new format you suggested:
-
What about adopting the same terms already established by other libs like react-transtion-group, instead of
inandout, we stick withenterandexit. You suggested this naming in the previous comments, and I think it's great because we reuse the same pattern already used by other libs. We could check if there are other commonly used libs that use another naming pattern. -
About the naming of the keyfram animation, I think it's not ideal to use
prevornext, the keyframe animation itself represents sliding in from left or right, or sliding out to the left or right, and it doesn't change, but with the CSS classes andrtlattribute we use different keyframes based on the case, the keyframe will still represent left or right transitions, but the class represents how to animate the elements based on the month entering the DOM being next or previous (after/before), and the text direction preference.
There was a problem hiding this comment.
@rodgobbi, you’re absolutely right. Let’s maintain the usage of “enter” and “exit” instead of “in” and “out,” and “left” and “right” for the keyframe names.
| cursor: pointer; | ||
| } | ||
|
|
||
| @keyframes rdp-animation_slide_in_left_keyframes { |
There was a problem hiding this comment.
I think we should shorten the keyframes names as:
| @keyframes rdp-animation_slide_in_left_keyframes { | |
| @keyframes rdp-slide_in_before { |
Reasoning - it's clear it belongs to animation and these are keyframes (no chance of "name collisions" to me).
(not sure for "left/right" yet)
There was a problem hiding this comment.
Discussed this in point 2 on the #2684 (comment)
| const { rootElRef } = useAnimation( | ||
| props, | ||
| classNames, | ||
| months, | ||
| focused, | ||
| dateLib | ||
| ); | ||
|
|
There was a problem hiding this comment.
Love this! It sounds strange to me that we need to return something here. Couldn't we initialize the root ref into the component instead?
I also propose a different arguments API:
| const { rootElRef } = useAnimation( | |
| props, | |
| classNames, | |
| months, | |
| focused, | |
| dateLib | |
| ); | |
| const rootElRef = useRef(); | |
| useAnimation(rootElRef, enabled, { classNames, months, focused, dateLib }); |
Looks more declarative to me.
There was a problem hiding this comment.
Makes sense 👍
the initial idea was to isolate the rootElRef in the hook, but for me it indeed makes sense to pass it as an argument.
|
|
||
| return ( | ||
| <components.Month | ||
| data-month-container={props.animate ? "true" : undefined} |
There was a problem hiding this comment.
| data-month-container={props.animate ? "true" : undefined} | |
| data-animated-month={props.animate ? "true" : undefined} |
Here and in the other data attributes, I would always use the animated- prefix instead of container because these data attributes are only valid in an animated calendar.
I like that we do not render them when not needed 🔝
There was a problem hiding this comment.
good idea 👍
it makes more clear it's for the animation
| describe("animate prop is falsy", () => { | ||
| it("should not change the default props", () => { | ||
| render(<DayPicker />); |
There was a problem hiding this comment.
I'd like to call "data attributes" the data- things added to the elements:
| describe("animate prop is falsy", () => { | |
| it("should not change the default props", () => { | |
| render(<DayPicker />); | |
| describe("animate prop is not set", () => { | |
| it("should not render any data-animated element", () => { | |
| render(<DayPicker />); |
There was a problem hiding this comment.
make sense, I'll rephrase a bit because not render any data-animated element looks like the element will not be rendered at all.
| it("should add data attributes if animate is true", () => { | ||
| render(<DayPicker animate={true} numberOfMonths={2} />); | ||
|
|
There was a problem hiding this comment.
| it("should add data attributes if animate is true", () => { | |
| render(<DayPicker animate={true} numberOfMonths={2} />); | |
| it("should render data-animated elements", () => { | |
| render(<DayPicker animate={true} numberOfMonths={2} />); | |
| /** CSS classes used for animating months and captions. */ | ||
| export enum Animation { | ||
| /** Applied to the entering month when it is after the exiting month. */ | ||
| animation_enter_month_weeks_is_after = "animation_enter_month_weeks_is_after", |
There was a problem hiding this comment.
I love everything but the verbosity here :) What about:
export enum Animation {
weeks_next_enter = "weeks_next_enter",
weeks_next_exit = "weeks_next_exit",
weeks_prev_enter = "weeks_prev_enter",
weeks_prev_exit = "weeks_prev_exit",
caption_next_enter = "caption_next_enter",
caption_next_exit = "caption_next_exit",
caption_prev_enter = "caption_prev_enter",
caption_prev_exit = "caption_prev_exit"
}
Idea from the board:
DayPicker Plans (view)
Description
Add a feature to animate the calendar changing the displayed month. The feature can be enabled by setting the prop
animatetotrue.The core idea in the approach of this implementation is to imperatively manipulate the DOM when the displayed months change, because doing it with only React APIs is very convoluted and has bad performance.
All the DOM manipulation happens inside a
useLayoutEffectto set up the animation right before the browser renders.There is a new set of CSS classes that are imperatively applied to the DOM to trigger the animations that are based on the calendar changing to a month that is after the previous month or before it because depending on this condition, the calendar weeks should animate left or right, and is inverted when the direction is
rtl.The animation is disabled when navigating the calendar with the keyboard arrows because focusing on the month being animated can cause issues with the animation. Animation can be worse for a11y, therefore disabling for keyboard navigation seems better.
Demo
Single.mov
Rtl.mov
Multiple.mov