[Popup][base-ui] Use context-based transition API #39326
Conversation
Netlify deploy preview@material-ui/unstyled: parsed: +1.76% , gzip: +1.50% Bundle size reportDetails of bundle changes (Toolpad) |
michaldudak
added
component: Popper
| }; | ||
| } | ||
|
|
||
| export function useTransitionableElement(requestEnter: boolean) { |
There was a problem hiding this comment.
Based on the usage in Popup.tsx, shouldn't this be a Transition component that contains the transition provider?
There was a problem hiding this comment.
Yup, could be as well. I wasn't focusing that much on the implementation as I wanted to check how would it feel to use it (from a developer's perspective). Do you have any remarks about that (especially in comparison to our current solution)?
There was a problem hiding this comment.
This is a significant improvement from the previous implementation 🎉
I left a couple of questions regarding naming and making the useTransition hook higher level, but overall I think this is the right direction.
Another thing we should consider is how our implementation interacts with the most common animation libraries like Framer Motion, React Spring, or tailwindcss-animate. For example, it seems to work good with React Spring, but I find it a little weird that the whole Popup is the one wrapped transitions rather than just the content. If there's anything we can do to make these libraries first-class citizens, we should.
As a side note, I like this implementation more than Radix's and React Aria's, which add a data- attribute for the transition state. Exposing that state through a hook gives more flexibility to work with whatever styling or animation solution is used.
| }; | ||
| } | ||
|
|
||
| export function useTransitionableElement(requestEnter: boolean) { |
There was a problem hiding this comment.
As an implementation detail, I think a reducer would be better for keeping the state if we go this route.
There was a problem hiding this comment.
I tried it in 9339c3f, but I'm not sure it's better. There's more to reason about and the code is much longer.
What do you think?
There was a problem hiding this comment.
I think it's better with a reducer as the hasEntered and inTransition are heavily related. With the reducer, it will be harder for them to become unsynced. It's a personal preference, though, and if there are not many changes to the hook in the future, my fear of them becoming unsynced by mistake is low and coverable by tests. I'll leave the decision to you 🤗
I added a React-spring example where it's not needed to wrap the whole Popup - the transition is defined as a child. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
|
I added the CssAnimation and CssTransition components so it's not necessary to deal with low-level events. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
|
@samuelsycamore, I'd appreciate your review of the docs. |
michaldudak
removed
the
proof of concept
docs/data/base/components/transitions/CssTransitionComponent.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Diego Andai <diego@mui.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: Michał Dudak <michal.dudak@gmail.com> Co-authored-by: Diego Andai <diego@mui.com> Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Adding new transition components and hooks and using them in the Popup component.
In contrast to the current implementation, this does not require the popup child to be a function. Because of this, it'll be possible to provide custom transition component using the
slotsprop.Related issue: #38280