-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Chrome Next] Add AppHeader to dashboards listing page #271945
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,11 +17,13 @@ import { useExecutionContext } from '@kbn/kibana-react-plugin/public'; | |
| import { QueryClientProvider } from '@kbn/react-query'; | ||
| import type { EmbeddableEditorBreadcrumb } from '@kbn/embeddable-plugin/public'; | ||
|
|
||
| import { AppHeader } from '@kbn/app-header'; | ||
| import type { AppMenuConfig } from '@kbn/core-chrome-app-menu-components'; | ||
| import { coreServices } from '../services/kibana_services'; | ||
| import { dashboardQueryClient } from '../services/dashboard_query_client'; | ||
| import { DASHBOARD_APP_ID, LANDING_PAGE_PATH } from '../../common/page_bundle_constants'; | ||
| import { getDashboardListingTabs } from './get_dashboard_listing_tabs'; | ||
| import type { DashboardListingProps } from './types'; | ||
| import type { DashboardListingProps, DashboardListingTab } from './types'; | ||
|
|
||
| export const DashboardListing = ({ | ||
| children, | ||
|
|
@@ -86,19 +88,64 @@ export const DashboardListing = ({ | |
| [tabs, activeTabId] | ||
| ); | ||
|
|
||
| const appMenu: AppMenuConfig = useMemo(() => { | ||
| const tabsByIdMap = new Map((tabs as DashboardListingTab[]).map((tab) => [tab.id, tab])); | ||
| return { | ||
| primaryActionItem: { | ||
| id: 'create', | ||
| testId: 'dashboardListingCreateButton', | ||
| iconType: 'plus', | ||
| label: i18n.translate('dashboard.listing.createButtonLabel', { | ||
| defaultMessage: 'Create', | ||
| }), | ||
| popoverWidth: 180, | ||
| items: [ | ||
| { | ||
| id: 'createDashboard', | ||
| order: 1, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Hehe, I still think it is weird that the API requires
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't always be the case that you will define all items in one file and having an if branch "if there's order, use it if not then use the order items were passed in" seems confusing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder what an example of this is and what it looks like in code. I understand But when it is a place where we pass the final order into the component, this seems confusing.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Take a look at how discover defines app menu - each button is defined in a separate file. It's easier to understand which button has which position when you see |
||
| label: 'Dashboard', | ||
| iconType: 'productDashboard', | ||
| testId: 'createDashboardButton', | ||
| run: () => tabsByIdMap.get('dashboards')?.createAction?.(), | ||
| }, | ||
| { | ||
| id: 'createVisualization', | ||
| order: 2, | ||
| label: 'Visualization', | ||
| iconType: 'chartBarVertical', | ||
| testId: 'createVisualizationButton', | ||
| run: () => tabsByIdMap.get('visualizations')?.createAction?.(), | ||
| }, | ||
| { | ||
| id: 'createAnnotation', | ||
| order: 3, | ||
| label: 'Annotation', | ||
| iconType: 'flag', | ||
| testId: 'createAnnotationButton', | ||
| run: () => tabsByIdMap.get('annotations')?.createAction?.(), | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| }, [tabs]); | ||
|
|
||
| return ( | ||
| <I18nProvider> | ||
| <QueryClientProvider client={dashboardQueryClient}> | ||
| {children} | ||
| <TabbedTableListView | ||
| headingId="dashboardListingHeading" | ||
| <AppHeader | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: Curious why we aren't upgrading the header inside the TabbedTableListView
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason really. I picked the first component that had
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I thought there was a good reason. because otherwise more 1-to-1 migration would be putting it into TabbedTableListView where PageTemplate wraps the header, so it would be my default choice |
||
| title={i18n.translate('dashboard.listing.title', { | ||
| defaultMessage: 'Dashboards', | ||
| })} | ||
| menu={appMenu} | ||
| /> | ||
| <TabbedTableListView | ||
| headingId="dashboardListingHeading" | ||
| getBreadcrumbs={getBreadcrumbs} | ||
| tabs={tabs} | ||
| activeTabId={activeTabId} | ||
| changeActiveTab={changeActiveTab} | ||
| showCreateButton={false} | ||
| /> | ||
| </QueryClientProvider> | ||
| </I18nProvider> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.