[base] Add `useModal` hook #38187

mnajdova · 2023-07-27T13:51:23Z

This PR extract the Modal's logic into a hook, that can be re-used in other components too, for e.g. Drawer. All modal's logic is moved to the hook, the only bits left in the component are the slots creation and the rendered elements. In the hook I used Base UI's notion for creating event handlers, the rest is the logic that was previously leaving on Joy UI's Modal component.

I didn't add tests for the useModal hook, as the Modal component was already covered for most of the logic.

mui-bot · 2023-07-27T13:54:34Z

Netlify deploy preview

docs/data/base/components/modal/modal.md

@material-ui/core: parsed: +0.15% , gzip: +0.21%
@material-ui/unstyled: parsed: +0.61% , gzip: +0.41%
@mui/joy: parsed: +0.20% , gzip: +0.25%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5ce713a

packages/mui-joy/src/Modal/useModal.ts

Signed-off-by: Marija Najdova <mnajdova@gmail.com>

packages/mui-joy/src/Modal/useModal.ts

oliviertassinari · 2023-07-29T21:19:06Z

packages/mui-joy/src/Modal/useModal.ts

+// Modals don't open on the server so this won't conflict with concurrent requests.
+const manager = new ModalManager();
+
+const useModal = (parameters: UseModalParameters) => {


To move in Base UI?

In the future yes, we will. The idea was to add it in Joy UI first and validate it, and later move it to Base UI. For now, I need it in #38169

Ok fair enough, I was thinking of exposing it as unstable from Base UI, but you own the Modal, so I think we are good, there is little risk that this gets duplicated with Material UI.

Things I find confusing:

A Joy UI Modal is not equivalent to a Base UI Modal, it's equivalent to a Material UI Dialog. As far as know, this is not the right terminology from an a11y standpoint. See the 3 different notions:

Dialog: https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/

Modal Dialog: https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/

Modal: https://en.m.wikipedia.org/wiki/Modal_window

Overall, I feel that Joy UI Modal contradicts what we say in https://mui.com/base-ui/react-modal/

The term "modal" is sometimes used interchangeably with "dialog," but this is incorrect. A dialog may be modal or nonmodal (modeless).

A Joy UI Modal doesn't have to block interactions with the rest of the page. How about we rename Joy UI Modal to Dialog?

The need for a hook. In Material UI component composition was enough. Is the objective to improve performance?

The initial need for the hook was adding a Drawer component in Joy UI. The Drawer is basically a modal with different transitions. I started with implementing it using component composition, however, we discussed that probably extracting the logic in a hook would be better, so that we avoid some of the problems we have in Material UI with component composition (see #38169) - this is why I initially opened the pull request. However, it seems better to have this hook on Base UI level, so that long-term Material UI will benefit it too, both in terms of performance and avoiding some of the problems that naturally comes from component composition. We can base all Modal-like component to use the hook instead of using component composition.

I am moving the hook to Base UI as unstable.

A Joy UI Modal doesn't have to block interactions with the rest of the page. How about we rename Joy UI Modal to Dialog?

THey block, this is exactly the same behavior as Base UI - the Modal component was literally copy pasted. The Modals both in Base UI and Joy UI interrupt interactions.

We discussed this with Olivier - here is the outcome:

✅ The Modal components work as expected both in Material UI, Base UI and Joy UI.

❓ The Joy UI's Modal page can likely be split into multiple pages, one talking about the modal and one talking about Dialogs (and alerts). It will be easier to distinguish what is modal vs dialog. cc @siriwatknp

❌ The biggest problem is the different behavior of the components that have some modals inside. For e.g. Material UI's Select dropdown is a modal - it blocks interactions with other elements, for e.g. [Menu] Clicking outside a menu blocks click from bubbling up #11243. This is the opposite behavior in Joy UI where the dropdown is non-modal. However, we should support both of these scenarios in both Material UI and Joy UI. What is most likely scenario is that on mobile the dropdowns need to be modal - so that clicking outside to close it does not unintentionally takes action. On desktop, it's likely most expected to have the non-modal scenario, as the screen is big enough to find a space to click without taking another action if that is the intention.

A Joy UI Modal is not equivalent to a Base UI Modal, it's equivalent to a Material UI Dialog

@oliviertassinari Can you elaborate? From my view, Joy UI Modal is the same as Base UI Modal. You can pass any children to create blocking interaction to let users focus on the content of the Modal.

Now Joy UI also provides a ModalDialog which follows https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/

Meaning, if you want to create a modal dialog based on above pattern:

<Modal> // <-- the modal which blocks interaction <ModalDialog> // <-- a container the building modal dialog … </ModalDialog> </Modal>

A Joy UI Modal doesn't have to block interactions with the rest of the page. How about we rename Joy UI Modal to Dialog?

Joy UI Modal should block interactions. What we need to do is create a new component called Dialog which will not block interaction by default and let developers integrate with their triggers.

The Joy UI's Modal page can likely be split into multiple pages, one talking about the modal and one talking about Dialogs (and alerts). It will be easier to distinguish what is modal vs dialog

Agree, we can consider splitting the page when we have a Dialog component for Joy UI.

@siriwatknp we discussed this offline with Olivier this comment #38187 (comment) summarized the conclusions. Everything is fine with the Joy UI's modal, we just likely need to split the page so that it's easier to consume :)

The other next thing would be supporting modal and non-modal version for all components that have some kind of drop downs, selects, autocomplete, menu etc.

However, we should support both of these scenarios in both Material UI and Joy UI.

Oh, yes, this would be so great 👍

We can consider splitting the page

This would make for a higher level of clarity 💯

What we need to do is create a new component called Dialog which will not block interaction by default and let developers integrate with their triggers.

I'm not sure, what would a "non modal dialog" look like? Something like this https://opensource.ebay.com/mindpatterns/messaging/toast-dialog/index.html?

I think that https://mui.com/joy-ui/react-modal/#alert-dialog is wrong, it shouldn't close when clicking on the backdrop. https://www.notion.so/mui-org/Alert-Dialog-should-close-on-backdrop-click-443f21d45852469bb3c71d94be6a57e6

…ial-ui into joy/useModal-hook

mnajdova · 2023-08-02T12:17:21Z

@michaldudak can you take a look at this? I will address #38187 (comment) separately - this PR just moved the logic for the modal into reusable hook that is reused in Base UI's and Joy UI's modal.

Next steps:

Implement Joy UI's Drawer using the hook
Move Material UI's Modal to use the hook
Resolve [base] Add useModal hook #38187 (comment)

packages/mui-base/src/unstable_useModal/useModal.ts

michaldudak · 2023-08-03T14:51:32Z

packages/mui-base/src/unstable_useModal/useModal.ts

+    // @ts-ignore internal logic - Base UI supports the manager as a prop too
+    manager = defaultManager,
+    closeAfterTransition = false,
+    onTransitionEnter,


We can unify the transition APIs between components in a separate PR.

packages/mui-base/src/unstable_useModal/useModal.types.ts

michaldudak · 2023-08-03T15:09:18Z

packages/mui-base/src/unstable_useModal/useModal.types.ts

+export type UseModalRootSlotProps<TOther = {}> = TOther & UseModalRootSlotOwnProps;
+
+export type UseModalParameters = {
+  'aria-hidden'?: React.AriaAttributes['aria-hidden'];


It's unusual to have an ARIA attribute on a hook. It would be great if it was described or renamed (or, even better, both).

Why would it be unusual to have it? If we don't have it, we would need to duplicate this logic three times and teach people how to process the prop before sending it to the hook under some other name. It's clear what it is. For e.g. react aria has this often, for e.g. https://react-spectrum.adobe.com/react-aria/useSelect.html.

I meant it's not usual in our code base. Usually (if not always) we place ARIA attributes based on some other parameters. Having a verbatim aria-hidden may suggest it's just forwarded as an element prop.

Having a verbatim aria-hidden may suggest it's just forwarded as an element prop.

I understand but why is this bad thing? This is what I am trying to understand. The hooks are intended to be used in components, so why is it bad thing to just forward some prop?

My point is that it may suggest it's just forwarded, but there is some logic associated with it, so it won't always end up being forwarded.
Plus, it's easier to specify hidden in the hook's parameters than 'aria-hidden'.

I feel like it's straight forward, let's see if we will have complains about it. The hook is marked as unstable_ we can change if we see opportunities to improve.

packages/mui-base/src/unstable_useModal/useModal.ts

michaldudak · 2023-08-08T12:35:53Z

packages/mui-base/src/unstable_useModal/useModal.ts

+    if (open && isTopModal()) {
+      handleMounted();
+    } else if (modalRef.current) {
+      ariaHidden(modalRef.current, ariaHiddenProp);


Can't we return the aria-hidden in getRootProps instead of using DOM methods directly?

This will cause another re-render. I wouldn't change the logic of it now. I am just extracting the logic in a hook. We can revisit and see if we can improve in a later iteration.

packages/mui-base/src/unstable_useModal/useModal.ts

siriwatknp

Overall, looks good to me.

Co-authored-by: Michał Dudak <michal.dudak@gmail.com> Signed-off-by: Marija Najdova <mnajdova@gmail.com>

michaldudak

All is good. We have different thoughts about 'aria-hidden` but it's just a matter of opinion, so I don't want this to block this PR. If you think the DX id good enough with your solution, feel free to merge it.

michaldudak · 2023-08-11T08:05:00Z

packages/mui-base/src/unstable_useModal/useModal.types.ts

+export type UseModalRootSlotProps<TOther = {}> = TOther & UseModalRootSlotOwnProps;
+
+export type UseModalParameters = {
+  'aria-hidden'?: React.AriaAttributes['aria-hidden'];


My point is that it may suggest it's just forwarded, but there is some logic associated with it, so it won't always end up being forwarded.
Plus, it's easier to specify hidden in the hook's parameters than 'aria-hidden'.

oliviertassinari

The code abstraction level looks great 👍

oliviertassinari · 2023-08-20T23:31:58Z

.eslintrc.js

+    // demos - proptype generation
+    {
+      files: ['docs/data/base/components/modal/UseModal.js'],


Maybe to generalize, if it's code generated, I guess this applies to everything.

[joy] Add useModal internal hook

2999d30

mnajdova added component: modal This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Jul 27, 2023

mnajdova commented Jul 27, 2023

View reviewed changes

packages/mui-joy/src/Modal/useModal.ts Outdated Show resolved Hide resolved

Update packages/mui-joy/src/Modal/useModal.ts

d459eef

Signed-off-by: Marija Najdova <mnajdova@gmail.com>

mnajdova mentioned this pull request Jul 27, 2023

[joy-ui][Drawer] Add Drawer component #38169

Merged

mnajdova requested review from siriwatknp and michaldudak July 27, 2023 15:25

mnajdova marked this pull request as ready for review July 27, 2023 15:25

oliviertassinari reviewed Jul 29, 2023

View reviewed changes

mnajdova added 3 commits July 31, 2023 08:12

Import ariaHidden from Base UI

8483b6f

Merge branch 'joy/useModal-hook' of https://github.com/mnajdova/mater…

a77fc9b

…ial-ui into joy/useModal-hook

wip useModal in base UI

fa82187

mnajdova marked this pull request as draft August 1, 2023 07:07

mnajdova changed the title ~~[joy] Add useModal internal hook~~ [base] Add useModal hook Aug 1, 2023

mnajdova added 3 commits August 1, 2023 10:11

Merge branch 'master' into joy/useModal-hook

8dc8ea9

docs:api

0307cb0

Fix propagation of custom event handlers

cb78f37

mnajdova added package: base-ui Specific to @mui/base and removed package: joy-ui Specific to @mui/joy labels Aug 2, 2023

mnajdova added 9 commits August 2, 2023 08:49

Define the hook's types

b28568f

Fix import name for unstable hooks

83277a7

Fix imports and circular dependency

3a1578f

Add hook demo

7979081

Fix isTopModal type & usage

283633c

demos fixes

06d0475

Add use client directive

40d5174

prettier

7bd4142

lint & docs:api

122cc4e

mnajdova marked this pull request as ready for review August 2, 2023 12:12

mnajdova requested review from michaldudak and siriwatknp and removed request for michaldudak and siriwatknp August 3, 2023 07:09

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

mnajdova added 2 commits August 3, 2023 15:04

Merge branch 'master' into joy/useModal-hook

0f6740b

docs:api

b8103b7

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 Aug 3, 2023

michaldudak reviewed Aug 3, 2023

View reviewed changes

mnajdova added 2 commits August 4, 2023 09:25

Merge branch 'master' into joy/useModal-hook

577ad5e

Review comments

e54dfda

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

mnajdova added 3 commits August 4, 2023 10:26

ref -> rootRef

ebe554d

fixes

707fe36

more fixes

f1c79bf

mnajdova requested a review from michaldudak August 7, 2023 10:08

michaldudak reviewed Aug 8, 2023

View reviewed changes

packages/mui-base/src/unstable_useModal/useModal.ts Outdated Show resolved Hide resolved

michaldudak reviewed Aug 8, 2023

View reviewed changes

packages/mui-base/src/unstable_useModal/useModal.ts Outdated Show resolved Hide resolved

siriwatknp reviewed Aug 9, 2023

View reviewed changes

mnajdova and others added 2 commits August 9, 2023 11:34

Update packages/mui-base/src/unstable_useModal/useModal.ts

1e7ced7

Co-authored-by: Michał Dudak <michal.dudak@gmail.com> Signed-off-by: Marija Najdova <mnajdova@gmail.com>

simplify condition

5ce713a

mnajdova requested a review from michaldudak August 9, 2023 10:31

michaldudak approved these changes Aug 11, 2023

View reviewed changes

mnajdova merged commit d410b47 into mui:master Aug 11, 2023
21 checks passed

oliviertassinari reviewed Aug 20, 2023

View reviewed changes

mj12albert mentioned this pull request Nov 22, 2023

[Modal][base-ui] Create the hook #38056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[base] Add `useModal` hook #38187

[base] Add `useModal` hook #38187

mnajdova commented Jul 27, 2023 •

edited by mj12albert

Loading

mui-bot commented Jul 27, 2023 •

edited

Loading

oliviertassinari Jul 29, 2023

mnajdova Jul 31, 2023 •

edited

Loading

oliviertassinari Jul 31, 2023 •

edited

Loading

mnajdova Aug 1, 2023 •

edited

Loading

mnajdova Aug 3, 2023

siriwatknp Aug 9, 2023 •

edited

Loading

mnajdova Aug 9, 2023

oliviertassinari Aug 20, 2023

mnajdova commented Aug 2, 2023

michaldudak Aug 3, 2023

mnajdova Aug 4, 2023

michaldudak Aug 3, 2023

mnajdova Aug 4, 2023

michaldudak Aug 8, 2023

mnajdova Aug 9, 2023

michaldudak Aug 11, 2023

mnajdova Aug 11, 2023

michaldudak Aug 8, 2023

mnajdova Aug 9, 2023

siriwatknp left a comment

michaldudak left a comment

michaldudak Aug 11, 2023

oliviertassinari left a comment

oliviertassinari Aug 20, 2023

[base] Add useModal hook #38187

[base] Add useModal hook #38187

Conversation

mnajdova commented Jul 27, 2023 • edited by mj12albert Loading

mui-bot commented Jul 27, 2023 • edited Loading

Netlify deploy preview

Bundle size report

Choose a reason for hiding this comment

mnajdova Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

mnajdova Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Aug 2, 2023

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

Choose a reason for hiding this comment

siriwatknp left a comment

Choose a reason for hiding this comment

michaldudak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[base] Add `useModal` hook #38187

[base] Add `useModal` hook #38187

mnajdova commented Jul 27, 2023 •

edited by mj12albert

Loading

mui-bot commented Jul 27, 2023 •

edited

Loading

mnajdova Jul 31, 2023 •

edited

Loading

oliviertassinari Jul 31, 2023 •

edited

Loading

mnajdova Aug 1, 2023 •

edited

Loading

siriwatknp Aug 9, 2023 •

edited

Loading