[popups] `onOpenChangeComplete` prop #1305

atomiks · 2025-01-08T10:57:05Z

Closes #1208

New alternative to #1235 to support both symmetric open/close cases, which has a more intuitive API.

Reworks useAfterExitAnimation to support both open/close animations
onOpenChangeComplete is called with open: boolean

mui-bot · 2025-01-08T10:59:58Z

Netlify deploy preview

https://deploy-preview-1305--base-ui.netlify.app/

Generated by 🚫 dangerJS against 30d8bad

netlify · 2025-01-10T02:25:03Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`ca3823b`
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/6780847a864eef0008694eee
😎 Deploy Preview	https://deploy-preview-1305--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-01-10T02:28:01Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`0a2f764`
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/67a1a875b6ca62000849af58
😎 Deploy Preview	https://deploy-preview-1305--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: atomiks <cc.glows@gmail.com>

packages/react/src/tooltip/root/TooltipRoot.test.tsx

mj12albert · 2025-01-23T06:04:54Z

packages/react/src/utils/useOpenChangeComplete.tsx

+ * Calls the provided function when the CSS open animation or transition completes.
+ */
+export function useOpenChangeComplete(parameters: useOpenChangeComplete.Parameters) {
+  const { open, change = 'close', ref, onComplete: onCompleteParam } = parameters;


It may be a bit more readable if this change param didn't have a default, but it probably doesn't matter outside of looking at this code in Github

anyway I tested onOpenChangeComplete in a bunch of our hero demos and it works well (they all use CSS transitions) 👍

CC @michaldudak to review the implementation as well 🙏

Was a small optimization since it's called in many of the root components

I agree with Albert, not having this parameter present when calling the hook makes it more confusing: we're calling useOpenChangeComplete, but its onComplete handles just the closing part.
TBH the change prop itself isn't super clear and I had to spend some time to understand what it's for. Would it be possible to remove it and make the hook handle both cases at the same time? The second parameter to runOnceAnimationsFinish could perhaps be derived from open?

Changed it so the caller with onComplete specified needs to check the change direction

packages/react/src/utils/useOpenChangeComplete.tsx

michaldudak · 2025-01-29T09:52:17Z

packages/react/src/tooltip/popup/TooltipPopup.tsx

    useTooltipRootContext();
  const { side, align } = useTooltipPositionerContext();

+  useOpenChangeComplete({


Why this can't be handled in useTooltipRoot, as the close animation?

The timing of the ref assignment means it needs to be called in the popup component to be accessible

michaldudak · 2025-01-29T09:55:16Z

packages/react/src/utils/useOpenChangeComplete.tsx

+ * Calls the provided function when the CSS open animation or transition completes.
+ */
+export function useOpenChangeComplete(parameters: useOpenChangeComplete.Parameters) {
+  const { open, change = 'close', ref, onComplete: onCompleteParam } = parameters;


I agree with Albert, not having this parameter present when calling the hook makes it more confusing: we're calling useOpenChangeComplete, but its onComplete handles just the closing part.
TBH the change prop itself isn't super clear and I had to spend some time to understand what it's for. Would it be possible to remove it and make the hook handle both cases at the same time? The second parameter to runOnceAnimationsFinish could perhaps be derived from open?

Co-authored-by: Michał Dudak <michal.dudak@gmail.com> Signed-off-by: atomiks <cc.glows@gmail.com>

Signed-off-by: atomiks <cc.glows@gmail.com>

michaldudak · 2025-01-29T13:37:47Z

packages/react/src/alert-dialog/popup/AlertDialogPopup.tsx

@@ -63,9 +63,10 @@ const AlertDialogPopup = React.forwardRef(function AlertDialogPopup(
  useOpenChangeComplete({
    open,
    ref: popupRef,
-    change: 'open',
    onComplete() {


The most straightforward and idiomatic way to use this would be to have a parameter on the onComplete callback that determines if the popup was opened or closed.

I thought of that, but it's not actually necessary because you can just use the outer variable that's passed as open. The useEventCallback ensures it's not stale.

atomiks added 2 commits January 8, 2025 21:53

[popups] Rename onCloseComplete to onOpenChangeComplete

20617a3

Consistency

d742934

atomiks added 4 commits January 8, 2025 22:01

proptypes

5f8acd2

Rework tests

c5719b8

docs:api

29225b8

Fix open animations

30d8bad

atomiks mentioned this pull request Jan 8, 2025

Revert "[popups] Add onCloseComplete callback" #1306

Merged

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2025

Merge

ca3823b

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2025

Merge

f1ed2b9

atomiks added 2 commits January 10, 2025 13:34

Use next tick

9610d3f

onOpenChangeComplete reference override

aec975d

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 14, 2025

atomiks force-pushed the feat/onOpenChangeComplete branch from 05afb45 to aec975d Compare January 14, 2025 00:37

atomiks added 2 commits January 14, 2025 11:37

Merge branch 'master' into feat/onOpenChangeComplete

d084778

Fixes

5582453

atomiks changed the title ~~[popups] Rename onCloseComplete to onOpenChangeComplete~~ [popups] onOpenChangeComplete prop Jan 14, 2025

atomiks marked this pull request as ready for review January 14, 2025 02:08

atomiks requested review from colmtuite and michaldudak as code owners January 14, 2025 02:08

Fix style indent

190df5d

zannager added component: dialog This is the name of the generic UI component, not the React module! component: popover The React component. labels Jan 15, 2025

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2025

Merge branch 'master' into feat/onOpenChangeComplete

7444ef6

Signed-off-by: atomiks <cc.glows@gmail.com>

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 23, 2025

mj12albert reviewed Jan 23, 2025

View reviewed changes

packages/react/src/tooltip/root/TooltipRoot.test.tsx Outdated Show resolved Hide resolved

mj12albert approved these changes Jan 23, 2025

View reviewed changes

atomiks added 2 commits January 23, 2025 17:38

Fix tests

be38b74

Remove non-spy

f7b81f5

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 28, 2025

michaldudak reviewed Jan 29, 2025

View reviewed changes

atomiks and others added 2 commits January 29, 2025 21:10

Update packages/react/src/utils/useOpenChangeComplete.tsx

920eb15

Co-authored-by: Michał Dudak <michal.dudak@gmail.com> Signed-off-by: atomiks <cc.glows@gmail.com>

Merge branch 'master' into feat/onOpenChangeComplete

4fa7671

Signed-off-by: atomiks <cc.glows@gmail.com>

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025

atomiks added 2 commits January 29, 2025 21:28

Rework implementation

aabb95f

Lint

81af0b1

michaldudak reviewed Jan 29, 2025

View reviewed changes

atomiks added 2 commits February 4, 2025 15:44

Merge branch 'master' into feat/onOpenChangeComplete

998a31d

Test for on mount calling

0a2f764

atomiks merged commit 58a9a13 into mui:master Feb 4, 2025
22 checks passed

atomiks deleted the feat/onOpenChangeComplete branch February 4, 2025 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[popups] `onOpenChangeComplete` prop #1305

[popups] `onOpenChangeComplete` prop #1305

atomiks commented Jan 8, 2025

mui-bot commented Jan 8, 2025 •

edited

Loading

netlify bot commented Jan 10, 2025

netlify bot commented Jan 10, 2025 •

edited

Loading

mj12albert Jan 23, 2025

atomiks Jan 23, 2025

michaldudak Jan 29, 2025

atomiks Jan 29, 2025

michaldudak Jan 29, 2025

atomiks Jan 29, 2025

michaldudak Jan 29, 2025

michaldudak Jan 29, 2025

atomiks Jan 30, 2025

[popups] onOpenChangeComplete prop #1305

[popups] onOpenChangeComplete prop #1305

Conversation

atomiks commented Jan 8, 2025

mui-bot commented Jan 8, 2025 • edited Loading

Netlify deploy preview

netlify bot commented Jan 10, 2025

✅ Deploy Preview for base-ui ready!

netlify bot commented Jan 10, 2025 • edited Loading

✅ Deploy Preview for base-ui ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[popups] `onOpenChangeComplete` prop #1305

[popups] `onOpenChangeComplete` prop #1305

mui-bot commented Jan 8, 2025 •

edited

Loading

netlify bot commented Jan 10, 2025 •

edited

Loading