-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
[core] Add useTransitionStatus
and useExecuteIfNotAnimated
Hooks
#396
Conversation
atomiks
commented
May 7, 2024
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a function and not a ref object?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
useUnmountIfNoAnimations
useCallIfNoAnimations
(well, since the supplied function can be anything)
return React.useMemo( | ||
() => ({ | ||
mounted, | ||
setMounted, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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));
}
}
useTransitionStatus
and useAnimationUnmount
HooksuseTransitionStatus
and useExecuteIfNotAnimated
Hooks
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.
All good!