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

Modals should be inside Portals #567

Open
huwshimi opened this issue Aug 5, 2021 · 5 comments
Open

Modals should be inside Portals #567

huwshimi opened this issue Aug 5, 2021 · 5 comments
Labels
P3 Triaged Issue has been reviewed as part of legacy backlog grooming (project P3). Priority: Low Triaged: new architecture Triaged, to be addressed in new architecture of the design system

Comments

@huwshimi
Copy link
Collaborator

huwshimi commented Aug 5, 2021

To prevent Modals being accidentally cut off by parent styles we should wrap the modal component in a Portal, like we do for contextual menus:

https://github.com/canonical-web-and-design/react-components/blob/master/src/components/ContextualMenu/ContextualMenu.tsx#L196

We'd need to figure out how to control the display of the Modal as the functions to display/hide the modal would be inside the component:

const { openPortal, closePortal, isOpen, Portal, ref } = usePortal({

@hatched
Copy link
Contributor

hatched commented Aug 6, 2021

In the Juju Dashboard We have a wrapper around the Modal component to do just this. You can see the component here and one example of the usage here

What you'll notice is that the Modal visibility is controlled by a parent. If you want to close it we use an externally supplied callback which modifies the state on the parent to close the model. I prefer this approach as the Modal should always have an 'owner'.

@cristinadresch
Copy link

@hatched @huwshimi do you know the direction of this, do you need a meeting for this?

@hatched
Copy link
Contributor

hatched commented Aug 12, 2021

nah, when the work is being tackled we can have a quick pre-imp.

@huwshimi
Copy link
Collaborator Author

Yeah, I'm not really sure what the right solution is for this. It's not really pressing as there are easy work arounds when using the component, just a nice-to-have feature at some point.

@bartaz
Copy link
Member

bartaz commented Sep 30, 2024

Triage: seems to be high effort with relatively low impact. This can be considered for the new architecture.

@bartaz bartaz added P3 Triaged Issue has been reviewed as part of legacy backlog grooming (project P3). Triaged: new architecture Triaged, to be addressed in new architecture of the design system labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Triaged Issue has been reviewed as part of legacy backlog grooming (project P3). Priority: Low Triaged: new architecture Triaged, to be addressed in new architecture of the design system
Projects
None yet
Development

No branches or pull requests

4 participants