-
Notifications
You must be signed in to change notification settings - Fork 856
Add FlyoutSystemMenu component #8851
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
Add FlyoutSystemMenu component #8851
Conversation
1538889
to
2ec3f5f
Compare
const newHistory = [...state.history]; | ||
|
||
if (state.activeFlyoutGroup) { | ||
newHistory.push(state.activeFlyoutGroup); | ||
newHistory.unshift(state.activeFlyoutGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the ordering of history items to be reversed (newest in the front) to simplify the rendering of the history popover.
a19f9d3
to
9891366
Compare
@tsullivan I think the answer is 'yes' but will |
9891366
to
b5a3212
Compare
@ryankeairns, yes there aren't any limitations against putting an If you are rendering the components declaratively, you can compose them this way:
The This PR currently updates the API for state management: if you want to open a "basic" flyout and have a flyout that looks exactly the way flyouts look in the current version, then you would call Note that this PR is still in draft mode because the API decisions may need to change. |
Thanks @tsullivan . Added the 'Closes [issue]' text to the PR description. Feel free to remove that if you feel it is not needed here. |
Updated the description, added screenshots. |
@ryankeairns from that issue, I see:
👍🏻 This PR does support custom actions that implementors want to show in the menu bar, if the developers render the flyout structure declaratively. But for developers that use state management system API:
I think this is good for now, as I need to avoid making the state management system too polished in this phase. Some API design changes are forthcoming to meet a set of refined Kibana developer requirements. Some terminology changes will come in through that as well. |
Understood. For those following along, we understand that the current Security flyout does have custom actions that we'll ultimately need to support in the managed setup. |
b65ce43
to
37fc2f6
Compare
/** | ||
* Title to display in top menu bar and in the options of the history popover | ||
*/ | ||
title: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clintandrewhall just want to point this field out specifically, because we will have to think about how titles will be displayed in the top menu bar and in the history popover with the replacement API.
historyFilter?: ( | ||
history: EuiFlyoutSessionHistoryState<FlyoutMeta>['history'], | ||
activeFlyoutGroup?: EuiFlyoutSessionGroup<FlyoutMeta> | null | ||
) => EuiFlyoutSessionHistoryState<FlyoutMeta>['history']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked @ryankeairns about this today for clarification. When the design idea for this came up, it was to limit the number of items in the history popover visually. Old items in the history should still be accessible by scrolling down or paging back. The idea actually wasn't to give developers a way to pull items out of the history or replace a given set of history items with another, or empty the history completely. So the historyFilter
mechanism as it is in this PR isn't actually needed at this point. (But maybe it doesn't hurt to leave it here).
}) => { | ||
const { onClose } = useContext(EuiFlyoutMenuContext); | ||
|
||
const styles = useEuiFlyoutMenuStyles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could you please use useEuiMemoizedStyles
instead? It passes euiTheme
as an argument to the given styles function, so useEuiFlyoutMenuStyles
could be pure.
packages/eui/src/components/flyout/sessions/managed_flyout_menu.tsx
Outdated
Show resolved
Hide resolved
Cherry-picked from PR elastic#8851 by tsullivan. This adds the EuiFlyoutMenu component and managed flyout menu functionality, including bug fixes, terminology updates, documentation, and stories.
I see the flyout menu bar does not appear on the In its most minimal state, it would show the close button. |
💚 Build SucceededHistory
cc @tsullivan |
💚 Build Succeeded
History
cc @tsullivan |
@tsullivan I updated the base to |
Closes #8873Summary
Adds a new
EuiFlyoutMenu
component that provides a standardized top menu bar for flyouts. Key features:EuiFlyout
andEuiFlyoutChild
componentsEnhances the flyout session API with:
openSystemFlyout
method for creating flyouts designed for flyout-to-flyout navigation (flyout sessions).historyFilter
prop inEuiFlyoutSessionProvider
GO_TO_HISTORY_ITEM
action for direct history accessOther changes:
flyoutReducer
where themeta
data from incoming actions needed to be merged with the past state. Caused by#8857
Why are we making this change?
Part of https://github.com/elastic/kibana-team/issues/1606 - SharedUX Flyout System
Screenshots
Demonstrating navigating deeply through history
Demonstrating the
historyFilter
Impact to users
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
@default
if default values are missing) and playground toggles