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

Support popover vs. modal differentiation #945

Open
Tracked by #4654
ohltyler opened this issue Aug 1, 2023 · 4 comments
Open
Tracked by #4654

Support popover vs. modal differentiation #945

ohltyler opened this issue Aug 1, 2023 · 4 comments
Labels

Comments

@ohltyler
Copy link
Member

ohltyler commented Aug 1, 2023

Current guidelines state that popovers can be used when not in the context of a flyout, but should be modals when embedded within a flyout. Ideally we have pre-made components that are commonly popovers (e.g., filter list as seen on Discover page), and provide out-of-the-box support of that to render within a flyout, such that the popovers are converted to modals.

@ohltyler
Copy link
Member Author

ohltyler commented Aug 1, 2023

cc @xeniatup

@KrooshalUX
Copy link
Contributor

KrooshalUX commented Aug 2, 2023

The pattern guidance that was given in regards to the quick-create flow in an OuiFlyout for the Anywhere project was to not utilize OuiPopover specifically for sub-flows within forms in OuiFlyout. It is more appropriate for a sub-flow to be handled within OuiModal. The issue that we ran into was that in the current full-page experience, the subflow is expressed as a popover. @xeniatup and I discussed revisiting the current implementation of the full create flow as well as other sub-flows are expressed in modals in full page experiences and not as popovers. We are evolving pattern guidance project-over-project.

There are legitimate other uses of OuiPopover within OuiFlyout - for example, if OuiFlyout contains OuiBasicTable and OuiFiilterGroup to filter the table - OuiFilterGroups base component is OuiPopover. Supporting this conversion would have unintended consequences.

As we work towards publishing design system pattern guidance (which is different than component library documentation), this can be clarified even further.

Given that clarification, @ohltyler , do you see us needing to support this conversion?

@lezzago
Copy link
Member

lezzago commented Aug 10, 2023

There are legitimate other uses of OuiPopover within OuiFlyout - for example, if OuiFlyout contains OuiBasicTable and OuiFiilterGroup to filter the table - OuiFilterGroups base component is OuiPopover. Supporting this conversion would have unintended consequences.

For this, could we have a lever to have it forced as a popover regardless what component it is under in? This would allow us to have a bit cleaner code and not duplicate many things. Especially since for any flows similar to the Anywhere project will essentially have to duplicate the code for the two different flows when there should really be no duplication needed there to support both flows.

@KrooshalUX
Copy link
Contributor

@lezzago I would like to review the specific use case with you and @xeniatup - I have a hunch that the popover in the full flow that is providing this issue with maintenance may be a pattern that needs to be updated as well.

Let's take a look at that before we proceed further on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants