Skip to content

[popups] actionsRef prop#1236

Merged
atomiks merged 33 commits intomui:masterfrom
atomiks:fix/js-anim
Feb 18, 2025
Merged

[popups] actionsRef prop#1236
atomiks merged 33 commits intomui:masterfrom
atomiks:fix/js-anim

Conversation

@atomiks
Copy link
Contributor

@atomiks atomiks commented Dec 27, 2024

Closes #1160

JavaScript animations page. I focused on Motion fairly exclusively. The actionsRef prop holds imperative methods, currently just unmount. action.current.unmount() is for when animating opacity doesn't work in other libraries for the most part, as opacity: 0.9999 is really the simplest solution.

Motion experiments

Remove keepMounted on portal child parts

Remove unused imports

Remove keepMounted prop in tests
@atomiks atomiks added the core label Dec 27, 2024
@mui-bot
Copy link

mui-bot commented Dec 27, 2024

Netlify deploy preview

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

Generated by 🚫 dangerJS against 49bf54c

const openRef = useLatestRef(open);

useEnhancedEffect(() => {
React.useEffect(() => {
Copy link
Contributor Author

@atomiks atomiks Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to regular useEffect as the timing lets Motion know the exit animation styles have been applied inside a single requestAnimationFrame when checking for element.getAnimations() when keepMounted=true

onFinished: handleUnmount,
});

React.useImperativeHandle(params.unmountRef, () => ({ unmount: handleUnmount }), [handleUnmount]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An API with a bunch of imperative methods could be useful in general, not just for this, so it could be named something more generic to future-proof it. This concept may be similar to using hooks, or component stores as in Ariakit. One difference is you can't "read" values in render, it's just for accessing internal methods/state inside event handlers & effects (since it's a ref).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
A couple of Material UI components use this pattern and call the prop action (like https://github.com/mui/material-ui/blob/master/packages/mui-material/src/ButtonBase/ButtonBase.d.ts#L13)

@github-actions 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 2, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 7, 2025
Signed-off-by: atomiks <cc.glows@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 7, 2025
@atomiks atomiks changed the title [POC] unmountRef prop [POC] action prop Jan 7, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 8, 2025
Signed-off-by: atomiks <cc.glows@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 10, 2025
@netlify
Copy link

netlify bot commented Jan 10, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit b7a095f
🔍 Latest deploy log https://app.netlify.com/sites/base-ui/deploys/67b3d81a006112000850349b
😎 Deploy Preview https://deploy-preview-1236--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@atomiks atomiks changed the title [POC] action prop [popups] action prop Jan 10, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 5, 2025
/**
* A ref to imperative actions.
*/
action?: React.RefObject<{ unmount: () => void }>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of the ref should be exported so it's easy for consumers to pass a correct type to React.useRef

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how should it be exported? Like Dialog.ActionsRef?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, should be fine like that.

*
* Documentation: [Base UI Menu](https://base-ui.com/react/components/menu)
*/
const MenuRoot: React.FC<MenuRoot.Props> = function MenuRoot(props) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit type definition was added in #705. Make sure the types are still generated correctly.

Copy link
Contributor Author

@atomiks atomiks Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I just noticed that DialogRoot didn't have this definition, but MenuRoot did. When I checked if the Root accepted arbitrary props it didn't (TS errors). However, based on the issues, I realize this may be exclusive to consumers of the built dist files, and not our dev environment. Do you remember if there was a difference there? (If so, Dialog must currently have this issue 🤔)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, DialogRoot has the same issue - it has propTypes in its built type definitions:

declare const DialogRoot: {
    (props: DialogRoot.Props): React.JSX.Element;
    propTypes: any;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed Roots that were missing it here

atomiks and others added 2 commits February 5, 2025 20:46
Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: atomiks <cc.glows@gmail.com>
@atomiks atomiks changed the title [popups] action prop [popups] actionsRef prop Feb 5, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 5, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 6, 2025
@mj12albert
Copy link
Member

🤔 I wonder if this is a viable method for cleanly closing menus in this situation: #1349 (comment)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 13, 2025
Signed-off-by: atomiks <cc.glows@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 18, 2025
@atomiks atomiks enabled auto-merge (squash) February 18, 2025 00:51
@atomiks atomiks disabled auto-merge February 18, 2025 00:52
@atomiks atomiks merged commit 961fd3e into mui:master Feb 18, 2025
20 of 21 checks passed
@oliviertassinari oliviertassinari added internal Behind-the-scenes enhancement. Formerly called “core”. and removed core labels Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[all components] JavaScript exit animations aren't properly supported

5 participants