[core] Add useTransitionStatus and useExecuteIfNotAnimated Hooks
#396
Conversation
atomiks
commented
May 7, 2024
atomiks
commented
- I have followed (at least) the PR section of the contributing guide.
| * @param isRendered - a boolean that determines if the component is rendered. | ||
| * @ignore - internal hook. | ||
| */ | ||
| export function useTransitionStatus(isRendered: boolean) { |
There was a problem hiding this comment.
How do you feel about returning props from this hook as well (onTransitionEnd and onAnimationEnd)? We may be able to omit setMounted from the returned object.
There was a problem hiding this comment.
I feel that should be an implementation detail of the components that use this hook instead. This keeps its responsibility quite minimal.
packages/mui-base/src/useTransitionStatus/useTransitionStatus.types.ts
Outdated
Show resolved
Hide resolved
atomiks
changed the title
useTransitionStatus HookuseTransitionStatus and useAnimationUnmount Hooks
| // Notes: | ||
| // - A single `requestAnimationFrame` is sometimes unreliable. | ||
| // - `queueMicrotask` does not work. | ||
| frame1Ref.current = requestAnimationFrame(() => { |
There was a problem hiding this comment.
I'm wondering if two animation frames are guaranteed to be reliable.
If we detect an animation where there isn't one (the [data-instant] example), we may end up with an element waiting indefinitely to be unmounted, breaking the user interface.
There was a problem hiding this comment.
Playing around with the transitions experiments demo with [data-instant], I wasn't ever able to reproduce it not unmounting. If there isn't any animation at all (no [data-instant] or anything), this won't be an issue at all.
| * @ignore - internal hook. | ||
| */ | ||
| export function useAnimationUnmount( | ||
| getElement: () => Element | null | undefined, |
There was a problem hiding this comment.
Why is this a function and not a ref object?
There was a problem hiding this comment.
Because of the wrapper in anchored popups, it's the firstElementChild of the floating element.
The usage is:
const unmount = useAnimationUnmount(
() => popupEl?.firstElementChild,
() => setMounted(false)
);
| * Unmounts the supplied element only if it has no animations or transitions. | ||
| * @ignore - internal hook. | ||
| */ | ||
| export function useAnimationUnmount( |
There was a problem hiding this comment.
I find it quite hard to understand the purpose of this function when reading the code. It doesn't really unmount anything (as the name would suggest), but it's a means of detecting if the element has any animation defined in its styles. Renaming it could make it easier to grasp.
There was a problem hiding this comment.
Yeah, I didn't really know what to call it without it being really long. It returns an unmount function, but it's simply guarded to only run if the element has no animations/transitions.
useUnmountIfNoAnimationsuseCallIfNoAnimations(well, since the supplied function can be anything)
| return React.useMemo( | ||
| () => ({ | ||
| mounted, | ||
| setMounted, |
There was a problem hiding this comment.
Does it make sense to call setMounted(true) from outside of this hook? If not, we could return an onAnimationEnd callback that will call setMounted(false) (and maybe also setTransitionStatus(undefined)?)
There was a problem hiding this comment.
setMounted gives them the freedom to perform more complex checks and keeps its responsibility minimal. For example, due to the wrapper in anchored popups, the checks in animationend/transitionend may differ. This gives full control.
Example:
function handleEnd({ target }: React.SyntheticEvent) {
const popupElement = refs.floating.current?.firstElementChild;
if (target === popupElement && setMounted) {
setMounted((prevMounted) => (prevMounted ? false : prevMounted));
}
}
atomiks
changed the title
useTransitionStatus and useAnimationUnmount HooksuseTransitionStatus and useExecuteIfNotAnimated Hooks
