[Chrome Next] Add AppHeader to dashboards listing page#271945
[Chrome Next] Add AppHeader to dashboards listing page#271945kowalczyk-krzysztof wants to merge 2 commits into
Conversation
fe7347e to
ac655f9
Compare
|
@kowalczyk-krzysztof, this PR increases one or more page-load bundle sizes by 15% or more:
Large bundle size increases can affect page load performance. Consider whether dependencies can be lazy-loaded or code split to reduce the bundle. See the bundle optimization guide for tips. |
8711af8 to
4cd49b2
Compare
4cd49b2 to
157b19f
Compare
| {children} | ||
| <TabbedTableListView | ||
| headingId="dashboardListingHeading" | ||
| <AppHeader |
There was a problem hiding this comment.
q: Curious why we aren't upgrading the header inside the TabbedTableListView
There was a problem hiding this comment.
No reason really. I picked the first component that had tabs defined already. We could move it one level down or even up but I think it's best for presentation team to decide on that.
There was a problem hiding this comment.
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
| items: [ | ||
| { | ||
| id: 'createDashboard', | ||
| order: 1, |
There was a problem hiding this comment.
nit: Hehe, I still think it is weird that the API requires order for the array of items :D
The array is the natural order in this case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I wonder what an example of this is and what it looks like in code.
I understand order for registry like api - registerItem({}) when you register from multiple places. There it makes sense.
But when it is a place where we pass the final order into the component, this seems confusing.
There was a problem hiding this comment.
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 order in that case. Also discover has a functionality built on-top of app menu (discover profiles) that lets you register custom app menu buttons, which is even more of a reason to keep order.
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
💛 Build succeeded, but was flaky
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
Summary
This PR migrates dashboard listing page to use the new
AppHeadercomponent as part Chrome Next project. TheCreate <variant>button that was previously one of the table action buttons, has been moved into app menu.Closes: https://github.com/elastic/kibana-team/issues/3421