search

opensearch-project / oui

OpenSearch UI Framework
Apache License 2.0
31 stars 65 forks source link

Support popover vs. modal differentiation #945

Open ohltyler opened 11 months ago

ohltyler commented 11 months ago

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 commented 11 months ago

cc @xeniatup

KrooshalUX commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

@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.