Skip to content
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

[base-ui][Select] Use Popup instead of Popper #40524

Merged
merged 17 commits into from
Jan 17, 2024

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Jan 10, 2024

Replaced the Popper with Popup in the Select implementation.
This also enabled the use of transitions in Select.

Some changes had to be made in the transitions infrastructure, as I encountered a few limitations:

  • Transitions now begin immediately after being requested (there's no need to call onEntering or onExiting callbacks anymore). This was required because the Select (and possibly other components) wanted to focus its child after being expanded. If we had to wait for the onEntering callback to be fired, the child could still be hidden by the time we needed it to be focused. This updated behavior is consistent with how Popper works.
  • Because of the above, the onEntering, onEntered, and onExiting callbacks were removed from the transition context and useTransitionTrigger hook. As a side effect, they are now simpler.

Breaking change

  • The popper slot was renamed to popup:
    -<Select slots={{ popper: StyledPopper }} slotProps={{ popper: { ... } }}>
    +<Select slots={{ popup: StyledPopup }} slotProps={{ popup: { ... } }}>
  • The slotProps.popup accept the props of the Popup component
  • The slots.popper used to be a Popper instance by default. Now, the slots.popup is a plain div and does not require the Popper/Popup instance to be passed in.

Part of #38280

@michaldudak michaldudak added component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Jan 10, 2024
@mui-bot
Copy link

mui-bot commented Jan 10, 2024

Netlify deploy preview

@material-ui/unstyled: parsed: -0.45% 😍, gzip: -0.29% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against a61059d

@michaldudak michaldudak force-pushed the transitions-in-select branch from 8c86aad to 02c57c3 Compare January 11, 2024 10:16
@michaldudak michaldudak force-pushed the transitions-in-select branch from 02c57c3 to 8053b35 Compare January 11, 2024 10:27
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Removing onEntering and onExiting makes sense to me, and appreciate the simplification, but I don't quite understand the removal of onEntered, isn't that still useful?

packages/mui-base/src/Select/Select.spec.tsx Show resolved Hide resolved
const { render } = createRenderer();
const { render: internalRender } = createRenderer();

async function render(
Copy link
Member

Choose a reason for hiding this comment

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

nit: render might be a confusing name as most tests use render returned by createRenderer directly. I would name this renderAndFlush

@@ -220,11 +220,11 @@ const Select = React.forwardRef(function Select<
)}
</Button>
{buttonDefined && (
<PopperComponent {...popperProps}>
<Popup slots={{ root: PopupComponent }} {...popupProps}>
Copy link
Member

Choose a reason for hiding this comment

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

It's incredible how surgical this replacement was. Congrats!

* If not specified, the transition will be considered finished end when the first property is transitioned.
* If all properties have the same `transition-duration` (or there is just one transitioned property), this can be omitted.
*/
lastTransitionedPropertyOnEnter?: string;
Copy link
Member

Choose a reason for hiding this comment

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

How were we able to remove this? What happens in case properties have different transitions for the enter transition?

@michaldudak
Copy link
Member Author

onEntered (and the related lastTransitionedPropertyOnEnter) isn't used, as there's no need to know when the entered transition ended. The only important event is onExited, as it tells when it's safe to unmount the transitioned element.

The sequence of entering goes like that:

requested enter -> mount (or remove visibility: hidden) -> start transition -> finish transition

Exiting:

requested exit -> start transition -> finish transition -> fire onExited -> unmount (or set visibility: hidden)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2024
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

The code looks good to me 😊

I did notice that if the popup placement (top/bottom) changes when hidden, then the appearing transition includes the "slide" from the previous position:

Screen.Recording.2024-01-16.at.10.30.52.mov

This is a detail though, only mention it in case we want to fix it, but I don't think it blocks the PR 😊

@michaldudak
Copy link
Member Author

Nice catch! I'll fix it.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2024
@michaldudak michaldudak merged commit ac5a1e0 into mui:master Jan 17, 2024
23 checks passed
@michaldudak michaldudak deleted the transitions-in-select branch January 17, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants