-
Notifications
You must be signed in to change notification settings - Fork 841
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
Regular flyout should work with the push flyout #7443
Comments
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. 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? |
Exactly, this is what I want!! |
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? |
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) |
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 sideIf these two flyouts:
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 sideIf these two flyouts:
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? |
I think you have described the cases perfectly Cee! At least for my case I need the second one:
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. |
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? |
@MichaelMarcialis thanx, my thoughts on that:
No this is not optimal, I want to see my Lens embeddable while I am editing it.
I am afraid that this is not possible at the scenario I examine. |
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. |
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. |
Michael I don't think that we have the space to do so in the inline editing 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. |
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! |
@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! |
Ok cool! Are you able to link to the code location of your workaround in Kibana here for our reference? |
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 |
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. elastic/kibana#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.
The text was updated successfully, but these errors were encountered: