-
-
Notifications
You must be signed in to change notification settings - Fork 20
feat(components): Popover exit animation #254
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
- Coverage 96.95% 93.27% -3.68%
==========================================
Files 70 152 +82
Lines 1542 2646 +1104
Branches 150 395 +245
==========================================
+ Hits 1495 2468 +973
- Misses 28 93 +65
- Partials 19 85 +66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Denny09310,
Thanks for this PR! This is something I’ve been meaning to get to for a long time but never quite managed.
I got a bit sick, so I couldn’t check it out earlier.
I have a couple of concerns:
- I don't think we need a new
AnimationEventArgs
at all; onanimationend
is already a valid event;- The popover should be removed from the DOM after exit;
- In general, I'd handle this just like I did with collapse (via intermediate states, such as expanded -> collapsing -> collapsed). Even then, the current solution isn’t ideal since the transition can’t be interrupted if the user spams clicks;
Because of this, I’d probably put this on hold for now.
What I really want is for the animation to be interruptible. For that, we should rely on transitions, since they are inherently interruptible. Keyframes, on the other hand, restart every time they’re triggered, which doesn’t look great.
What do you think?
I think it’s perfect. This was just the first approach I came up with, and I wanted your opinion on it. I haven’t found a better method to add an animation that also removes the element from the DOM afterward, but I like your idea more. To have the ability to interrupt the animation would be ideal |
Great! You might have noticed that I created a Blazor port of React Sonner. I went with So the flow would look something like this:
|
State = PopoverState.Opening; | ||
await InvokeAsync( StateHasChanged ); | ||
|
||
await Task.Delay( 100 ); // <-- I don't know how to avoid this. The animation works from the second time forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see this comment. I don't know how to solve it
Description
Add the ability to have a popover exit animation.
What's been done?
Checklist
Additional Notes
There is one test that the dropdown is failing even though visually it works well.