elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.1k stars 840 forks source link

Regular flyout should work with the push flyout #7443

Open stratoula opened 9 months ago

stratoula commented 9 months ago

Is your feature request related to a problem? Please describe.

There are cases where in Kibana you have a regular flyout and you want to open a push flyout from a CTA. I am expecting that the push flyout should move the regular flyout to the left in order both contents to be visible. Right now, this is not possible. The only thing that works is opening a regular flyout above the already existing one but this is not useful as in the majority of the cases we want the content from both flyouts to be visible

Use case One of the things that got released in kibana 8.11 and is going to be added in various places in the short term is the in-app editing of Lens embeddables. https://github.com/elastic/kibana/pull/166169 This allows the users to edit their Lens embeddables by opening a push flyout. There is no need to go to Lens which I feel makes sense in the majority of cases and is going to increase the ux a lot. One of the problems we have on integrating this to other apps is that sometimes an embeddable can live in a flyout (a regular one). Opening a push flyout from a regular flyout doesnt work

I know that there are already some discussions on allowing the regular flyout to have additional columns but I wonder if making the 2 types of flyouts work nicely together is a faster solution.

julianrosado commented 9 months ago

Just to reframe your problem so we are sure we're thinking the same thing:

There's two types of flyout, overlay (which goes over the underlying content) and push (which is part of the existing content and pushes everything over)

You have been successful in using push to create a "Setting" panel that allows a user to inline edit their dashboard's lens embeddables, as seen in your linked issue.

CleanShot 2024-01-10 at 14 19 02

However, sometimes the embeddable that a user would want to edit exists inside an overlay flyout. Since the overlay flyout is above the content, you no longer have the ability to see that settings panel.

What you want is the ability to open up the overlay flyout with the lens embeddable, but then allow a user to edit it by also open up a push flyout panel that pushes it over. Let me know if I got it!?

IF that is correct, I think that makes sense. @MichaelMarcialis you had said earlier that you consider the flyout as a modal experience, which I largely agree with, but in this case the context changed to it being a... wysiwyg editor? If we want to make editing something a user does in context of their live setup (vs having a separate, purpose-built page to edit embeddables) then having an additional, toggle-able column in the overlay flyout makes sense to me.

Whatcha think?

stratoula commented 9 months ago

What you want is the ability to open up the overlay flyout with the lens embeddable, but then allow a user to edit it by also open up a push flyout panel that pushes it over. Let me know if I got it!?

Exactly, this is what I want!!

stratoula commented 9 months ago

Something like that

example

cee-chen commented 9 months ago

The screencap you posted is super helpful, thanks Stratoula! My concern here with two flyouts on the same page at once is that each flyout is considered a dialogue and is focus trapped. AKA, there would be no way for keyboard/SR users to switch between the two different flyouts/panels, which feels like an accessibility issue. If both sections are meant to be interactable at once, it feels like what you're really looking for is another panel within the same flyout.

Just wanted to loop @boriskirov and maybe @CoenWarmer in here too for their thoughts - does this feel more like the "stackable flyout" concept that y'all were talking about?

stratoula commented 9 months ago

I am aware of the stackable flyout discussion and they are related of course. The reason I raised this is because the inline editing flyout comes from another plugin so it would be very easy for the consumers to make it work if the 2 types of flyouts were working together.

The stackable flyouts is a solution but still requires a different implementation for the consumers who have a Lens embeddable in an overlay flyout. My request is mostly for the sake of a simpler api and in my mind these 2 components should work together (I mean the push and the overlay flyout)

cee-chen commented 9 months ago

the inline editing flyout comes from another plugin so it would be very easy for the consumers to make it work if the 2 types of flyouts were working together.

I see that as an API issue to solve rather than a UI one. If necessary, the plugin should simply be adjusted to allow not rendering an entire flyout, but instead allow rendering flyout content (so that it can be injected into an existing flyout). A flyout should not be treated simply as a visual effect but as a contextual one, since it determines thing like focus traps and dialogues for non-sighted users.

TBH, I'm still kinda struggling mentally with delineating the use cases around multiple flyouts existing at once, so let me make sure I understand the desired user flow:

1. Two related flyouts need to exist side by side

If these two flyouts:

  1. Are closely related to one another
  2. Cannot be closed individually (i.e. closing the original flyout closes the new flyout)
  3. Need to share focus (users should be able to interact with/tab between both flyouts at once)

Then this is a stackable "flyout" scenario, which should essentially be a conditional extra vertical pane in the existing flyout + extending its width. On mobile, the two panes stack vertically and should both be reachable via scroll.

In this scenario, it does not make sense to render two flyouts with two separate DOM contexts & focus traps.

2. Two separate flyouts need to exist side by side

If these two flyouts:

  1. Are not closely related or dependent (e.g. the new flyout is higher level or related to the concept of the page or product as a whole)
  2. Can be closed individually (closing original flyout does not close the new one, and vice versa)
  3. Can but do not need to share focus (ideally, the new focus would receive its own focus trap)

This this is essentially a "super push flyout" (e.g., an AI assistant pane) and it makes sense for it to push the rest of the page, including existing flyouts. On mobile, it should overlay all other flyouts/the page completely.

Am I way off on this thinking?

stratoula commented 9 months ago

I think you have described the cases perfectly Cee! At least for my case I need the second one:

If necessary, the plugin should simply be adjusted to allow not rendering an entire flyout, but instead allow rendering flyout content

Correct, and this is definitely possible. The problem is that the consumers need to give more TLC which can be avoided if the 2 flyouts could work together as you describe on the second case.

MichaelMarcialis commented 9 months ago

@MichaelMarcialis you had said earlier that you consider the flyout as a modal experience, which I largely agree with, but in this case the context changed to it being a... wysiwyg editor?

Yes, I do consider flyouts to be a modal experience. The only exception to this is if it is a push flyout (which the Lens inline editing interface is, in this case). The push flyouts are an oddity that can create a lot of inconsistency with how flyouts are utilized. Additionally, I feel the push prop probably shouldn't be part of the EUI component. A push flyout largely operates as simple show/hide content, which can already easily be handled with basic React conditionals. I don't think abusing and bastardizing the flyout component (and it's intended modal experience) is needed to achieve such an experience.

My personal thoughts and philosophies on flyouts aside, the point remains that content meant to exist in the flow of the page (via push flyout or otherwise) just don't work well in tandem with a typical overlay flyout. My initial instinct is to say that supporting the display of two flyouts simultaneously seems like a bit of a heavy-handed approach (and one that might open a can of worms we don't want to open). Here's a few quick potentially cleaner approaches that come to mind:

Thoughts?

stratoula commented 9 months ago

@MichaelMarcialis thanx, my thoughts on that:

If the visualization the user desires to customize is housed within an overlay flyout, perhaps clicking an "edit visualization" button swaps out the existing content of the overlay flyout and temporarily replaces it with the Lens inline editing content.

No this is not optimal, I want to see my Lens embeddable while I am editing it.

Restructure the page so that the content in the current overlay flyout (with the visualization the user desires to customize) exists within the flow of the page (via push flyout or otherwise). If it exists in the flow of the page, additional content that exists in the flow of the page (such as the Lens inline editor) can also be added adjacent to it without being obscured.

I am afraid that this is not possible at the scenario I examine.

MichaelMarcialis commented 9 months ago

No this is not optimal, I want to see my Lens embeddable while I am editing it.

Agreed. I had mentioned in that comment that we'd need to account for the inclusion of a visualization preview as part of this experience, which may mitigate that concern. Not ideal, but better than multiple flyouts, in my mind.

JasonStoltz commented 9 months ago

One common sentiment I think we're going to encounter any time we get into discussions that involve multiple flyouts is this:

Are we putting too much content into flyouts? And are we creating a poor user experience by doing so?

The need for multiple flyouts to be visible at once could be a sign that the UX we are trying to build is too complex for a flyout.

stratoula commented 9 months ago

for the inclusion of a visualization preview as part of this experience, which may mitigate that concern.

Michael I don't think that we have the space to do so in the inline editing flyout. :/

The need for multiple flyouts to be visible at once could be a sign that the UX we are trying to build is too complex for a flyout.

I agree on that. But the scenario I am referring to is about the ai assistant. It is integrated in all the obs applications and it makes sense to me to be on an overlay flyout. I am not sure if the team had tried a push flyout instead and decided it against it. I dont want to talk for them.

We can't also change the UI of all the applications that are going to use the assistant 🤷‍♀️ We need solutions that can easily be adopted everywhere.

Regardless if it is a push or an overlay flyout I personally see the necessity sometimes for specific scenarios to be able to expand the flyout to give more functionality to the users.

Kibana applications in general are very complex. We all know that. They give a lot of functionality and many times we feel that we don't have space. Discover is a very good example. Flyouts are really helpful in these scenarios. I am mostly in favor of push flyouts. Overlay flyouts are cool but they hide the context. Regardless of how we name them, in my mind it is a sidebar which I decide if I want this to be visible or not.

With that being said I feel that this is something we need to support. At least for the case of an assistant which can be present in whichever application and help the users to accomplish some tasks.

If you don't feel in favor of it, it is also ok by me. I have found a way to solve this limitation.

JasonStoltz commented 5 months ago

Hey @stratoula, sorry for the radio silence on this one. Just wanted to circle back to you on this one.

We have this labeled as "High Priority" for the team. Just wanted to re-gauge the priority here. Do you have or are you anticipating any upcoming product needs that you're aware of that you'll need this for? Trying to figure out how to prioritize this relative to other work.

Thanks!

stratoula commented 5 months ago

@JasonStoltz fantastic. We found a workaround on the obs ai assistant but I am not very fond with our solution so if we could simplify the code and make it work natively from EUI it would be great. Our hacky solution has some bugs and some limitations so I would appreciate if we can prioritize it ❤️ Thanx a ton!

JasonStoltz commented 5 months ago

Ok cool! Are you able to link to the code location of your workaround in Kibana here for our reference?

stratoula commented 5 months ago

Sure https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/observability_ai_assistant_app/public/components/chat/chat_flyout.tsx#L143 here it is. If you have any questions please let me know. There is a complexity mostly because of this limitation