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

[Popover] Add interactable attribute #25271

Closed
wants to merge 3 commits into from

Conversation

yehee
Copy link

@yehee yehee commented Mar 9, 2021

As addressed in #25257, it is inherent as a modal to block all interactions with the rest of the page. The interactable flag will enable user interaction with the rest of the page when the popover is open.

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 9, 2021

Details of bundle changes

Generated by 🚫 dangerJS against d8a93e9

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.

This is not flying for the same reason as before. It should remove the Modal behavior all together.

@yehee
Copy link
Author

yehee commented Mar 9, 2021

@oliviertassinari Thanks for looking into it! Could you elaborate more on what it means by that? I've gone through Modal API doc to see if I can pick up on something, but I'd appreciate it if there are resources that I could refer to as well!

@oliviertassinari
Copy link
Member

If we want to disable the modal behavior, then I think that we should not mount the Modal component in the first place.

@yehee
Copy link
Author

yehee commented Mar 10, 2021

@oliviertassinari I see! Would you have any suggestions as to what would be the most appropriate component to replace in this case? I see that Modal is the component passed in experimentalStyled for root which can be easily swapped with another tag such as div, but it lacks a few properties defined in ModalUnstyled.d.ts that Popover depends on such as open or BackdropProps.

@eps1lon
Copy link
Member

eps1lon commented Mar 10, 2021

I don't think the Popover is the right component for the UX you want to achieven. Have you tried using the Popper instead?

@yehee
Copy link
Author

yehee commented Mar 12, 2021

@eps1lon Thanks for the suggestion! Popper was the other component I tried to work with, and I had an issue with positioning the component relative to the anchorEl. Since the anchorEl element I'm working with is dynamically rendered, it fails to detect the position of the anchor element and thus the popper does not get positioned properly. Popover had a similar issue, but they provide us with a workaround with anchorPosition which allows us to specify the position of the popover. I'm very new to the community, so I may have looked over some other alternatives as to working with Popper with dynamically rendering components.

@eps1lon
Copy link
Member

eps1lon commented Mar 15, 2021

Popover had a similar issue, but they provide us with a workaround with anchorPosition which allows us to specify the position of the popover.

Popper also allows setting the anchorEl to a "virtual" one i.e. setting the position manually. But there may be a bug with Popper. Could you file an issue so that we can take a look?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 19, 2021
@oliviertassinari oliviertassinari deleted the branch mui:next June 22, 2021 00:28
@oliviertassinari oliviertassinari changed the base branch from foo to next June 22, 2021 00:49
@eps1lon
Copy link
Member

eps1lon commented Jun 22, 2021

Closing since there hasn't been any response in 3 months to #25271 (comment)

@eps1lon eps1lon closed this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants