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

[docs][Dialog] Add non-modal dialog docs & demo #38684

Merged
merged 16 commits into from
Sep 18, 2023

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Aug 28, 2023

One of #38630.

In Material UI there is no documentation around non-modal dialogs. The docs states "A Dialog is a type of modal window that appears in front of app content to provide critical information or ask for a decision. Dialogs disable all app functionality when they appear, and remain on screen until confirmed, dismissed, or a required action has been taken.", but there are also non-modal dialogs scenarios, that would be great for us to document to help people build, for e.g. check https://accessuse.eu/en/non-modal-dialogs.html#:~:text=A%20non%2Dmodal%20dialog%20pops,partly%20covered%20by%20the%20dialog. This page has great examples of both use-cases https://www.nngroup.com/articles/modal-nonmodal-dialog/

This PR adds a non-modal section on the dialog page explaining the use-case and how can one build it.

Demo link: https://deploy-preview-38684--material-ui.netlify.app/material-ui/react-dialog/#non-modal-dialog

image

@mnajdova mnajdova added docs Improvements or additions to the documentation component: dialog This is the name of the generic UI component, not the React module! labels Aug 28, 2023
@mui-bot
Copy link

mui-bot commented Aug 28, 2023

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 72c53d2

@mnajdova mnajdova marked this pull request as ready for review August 28, 2023 12:37
@mnajdova mnajdova requested review from oliviertassinari, DiegoAndai, danilo-leal, samuelsycamore and a team and removed request for DiegoAndai and danilo-leal August 28, 2023 12:37
@danilo-leal
Copy link
Contributor

Sweet, this is a super helpful distinction/demo to be added!

Overall, about the docs ⎯ I'd love to hear @samuelsycamore's take on this one ⎯ I think the modal vs. non-modal explanation should probably not be inside a callout. Whenever content gets too big inside of them, I start to feel like they deserve a paragraph on their own. Callouts, to me, sound like extra information. That said, it should probably be inside the "Non-modal" section? Might also be interesting to link the NN Group link for further reference, it's indeed a great resource!

All in all, a Material UI docs information hierarchy similar to what we're doing with Base UI would be really beneficial to find these sections quickly!

@mnajdova
Copy link
Member Author

I think the modal vs. non-modal explanation should probably not be inside a callout. Whenever content gets too big inside of them, I start to feel like they deserve a paragraph on their own. Callouts, to me, sound like extra information. That said, it should probably be inside the "Non-modal" section? Might also be interesting to link the NN Group link for further reference, it's indeed a great resource!

Makes total sense, I am on it!

@mnajdova
Copy link
Member Author

@danilo-leal I added a cookie banner example too, please check if we want to adjust the styles there! :)

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Pushed a few changes, mostly simplifying the copywriting and demos. I think having two demo blocks is a bit overkill 😬 just one seems enough to demonstrate the idea. Also positioned the "Non-modal dialog" content a bit up in the page's hierarchy (not following any specific order, though, but thought that being below "Limitations" was a bit off).

Hope this is ok! It's looking great to me! 😃

@danilo-leal danilo-leal added the package: material-ui Specific to @mui/material label Aug 30, 2023
@mnajdova
Copy link
Member Author

Hope this ok! It's looking great to me! 😃

Thanks! Let's wait on Sam's final review.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have noticed one blocker:

  1. The rest of the page is not interactive, this is a deal breaker IMHO. This is a modal dialog behavior. I think this would be also true if it was a chatbot.

Otherwise:

  1. It sets aria-hidden="true" on all the elements of the page, this feels wrong.
  2. We want people to click "Allow all", I don't think we can have the least favorable option "Use necessary only" to take the most space on the screen (so the easiest to click). Personally, when I see these banners, I usually click on whatever hides it the fastest. Examples:

Ashby
Screenshot 2023-08-31 at 00 30 16

Stripe
Screenshot 2023-08-31 at 00 30 11

Vercel
Screenshot 2023-08-31 at 00 31 31

  1. In terms of where the accept button is positioned, I think https://vercel.com/ has the best design. The closest to the footer on mobile and the closest to the center on desktop, it's likely the least effort to click.
  2. The design has issues on desktop: https://tqptrk.csb.app/

@danilo-leal
Copy link
Contributor

@oliviertassinari Wasn't necessarily looking for the best of the best cookie banner design for this demo 😅 but I tweaked it a bit to be even simpler + fixed the desktop issue there. Also, I just noticed the apparently modal behavior (e.g., can't select text on the page), which we should probably fix as this is a non-modal demo. 😬

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 30, 2023

True, but we can have a great one 😁, these https://tailwindui.com/components/marketing/elements/banners#component-c5bc8a46a1093693fd286f0b74c631f4 aren't bad at all.

Speaking of great:

  • The iframe doesn't load the Roboto font, the font we see is sans-serif on macOS
Screenshot 2023-08-31 at 01 25 04

it seems to be an old issue, I can see the same issue on https://v4.mui.com/components/app-bar/#hide-app-bar.

  • I guess we should use <Container> for the body of the page

@mnajdova
Copy link
Member Author

The rest of the page is not interactive, this is a deal breaker IMHO. This is a modal dialog behavior. I think this would be also true if it was a chatbot.

I will look into this, in the end it may be best to have a different component instead of modal for the root element if the dialog should be non-modal - we can have this implemented either as prop or a different component. I will start with prop to see how big of a change it would be.

@oliviertassinari
Copy link
Member

@mnajdova I suspect that composing Paper, with Fade and TrapFocus is enough for this. No need for Dialog.

@mnajdova
Copy link
Member Author

@mnajdova I suspect that composing Paper, with Fade and TrapFocus is enough for this. No need for Dialog.

I was testing this a bit, but it's not that straight forward. The TrapFocus does not allow all the functionalities we would need - for e.g. I couldn't set it up to have circular navigation but also allow focusing/taking action by clicking outside of it at the same time. It's not that trivial, maybe we can track the last event and if it is a click outside of it - disable the FocusTrap. I will experiment with this for a bit and report back. I agree it would likely be outside of the scope of the Dialog, but we can likely keep the demo on this page.

@mnajdova mnajdova marked this pull request as draft August 31, 2023 12:04
@mnajdova
Copy link
Member Author

I am converting to draft, I will ask for a review when I have something better. Btw, I had a false self-approval that it works as the demo was iframe so clicking outside of the demo worked as expected 🤦‍♀️

@mnajdova mnajdova marked this pull request as ready for review September 1, 2023 10:54
@mnajdova
Copy link
Member Author

mnajdova commented Sep 1, 2023

I am happy with the demo now, it's ready for the next round of review. @oliviertassinari you were right about using Fade, TrapFocus and Paper, the tricky part was when to enable/disable the TrapFocus - I've added a focusin event listener for this.

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for working on this 🤙

The only thing, though, and this can be unrelated, is that the demo typography looks bold 😬 Don't think it's supposed to be like this, but not sure if it's something with this demo in specific or else.

Screen Shot 2023-09-01 at 14 15 10

@mnajdova
Copy link
Member Author

mnajdova commented Sep 4, 2023

@oliviertassinari your changes broke the keyboard behavior - the focus is no longer trapped inside the Cookies banner, and one more thing - there are two more focus stops - the TrapFocus's sentinel start and end. For e.g. check how the behavior on https://www.nngroup.com/articles/modal-nonmodal-dialog/ or gmail's email popup is different.

From the resources I found, there are the requirements:

  • ✅ allow interactions outside of it, for e.g. clicking outside of the non-modal dialog should allow selection/focus
  • ✅ correct aria attributes: role="modal", aria-modal="false", aria-label to be specified
  • ❌ once the focus is inside the non modal dialog it should be circulating inside (needs to be trapped) - this is broken by your last commit, but was working previously - I can revert the changes once we agree
  • ❓ provide some keyboard action that will allow users to focus outside of it (I see F6 as a recommendation in the AcessUse acrticle) - I am not sure what should be the recommendation, maybe we want to make users of the website take an action instead allow/disallow the cookies - this can likely be a decision made on an app level)

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 4, 2023

your changes broke the keyboard behavior - the focus is no longer trapped inside the Cookies banner, and one more thing - there are two more focus stops - the TrapFocus's sentinel start and end. For e.g. check how the behavior on https://www.nngroup.com/articles/modal-nonmodal-dialog/ or gmail's email popup is different.

@mnajdova Ah yes, I wasn't clear. It's what I described with "Enable the trap focus, deferring the cycling issue to #21569." I don't think the root issue is the demo, but the underlying component.

@mnajdova
Copy link
Member Author

mnajdova commented Sep 5, 2023

@mnajdova Ah yes, I wasn't clear. It's what I described with "Enable the trap focus, deferring the cycling issue to #21569." I don't think the root issue is the demo, but the underlying component.

Aah, I see now, thanks for the clarification. I will look into this issue first, looks like we need to have similar logic to what I had in the demo, unless something else broke it.


PR created for fixing the FocusTrap issue here #38816

@mnajdova mnajdova force-pushed the docs/dialog-non-modal branch 2 times, most recently from f93ee5d to f7dba80 Compare September 8, 2023 06:13
@mnajdova mnajdova force-pushed the docs/dialog-non-modal branch from f7dba80 to aefc6cd Compare September 8, 2023 06:18
@mnajdova
Copy link
Member Author

mnajdova commented Sep 8, 2023

@samuelsycamore this is ready for final review in terms of copywriting.

@samuelsycamore
Copy link
Contributor

samuelsycamore commented Sep 11, 2023

Sorry I missed this before! I'm not sure if anyone else already pointed this out, but the Base UI Modal doc includes this callout:

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

A modal [blocks interaction with the rest of the application](https://en.wikipedia.org/wiki/Modal_window), forcing the user to take action. 
As such, it should be used sparingly—only when the app requires user input before it can continue.

Maybe we should use the same text here? I suppose the purpose is a little different here, but I like this explanation.

@mnajdova
Copy link
Member Author

Maybe we should use the same text here? I suppose the purpose is a little different here, but I like this explanation.

We have the same callout on the Materia UI's modal page: https://mui.com/material-ui/react-modal/ Did you mean that we should duplicate this on the Dialog page too?

@mnajdova mnajdova merged commit 436801b into mui:master Sep 18, 2023
@oliviertassinari
Copy link
Member

The end result feel great 👌

danilo-leal pushed a commit to siriwatknp/material-ui that referenced this pull request Sep 18, 2023
christophermorin pushed a commit to christophermorin/material-ui that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dialog This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants