Page layouts/stage 2/stack management #100085
Conversation
# Conflicts: # src/core/public/rendering/_base.scss
(KibanaPageLayout -> KibanaPageTemplate)
# Conflicts: # src/plugins/kibana_react/public/page_layout/page_layout.tsx
Todo: Fix pageTitle icon and supply bottom bar to layout when upgrading EUI
| </EuiPageBody> | ||
| <ManagementPageLayout | ||
| pageHeader={{ | ||
| pageTitle: ( |
There was a problem hiding this comment.
If pageTitle is a fairly consistent element in this layout, then I think it makes sense to hard-code a data-test-subj to it for reliable identification in tests. Maybe something unique, like data-test-subj="page-layout-title"? We can update the tests to depend upon this selector value instead of the existing "appTitle".
There was a problem hiding this comment.
👍 I added/hard-coded this in the KibanaPageTemplate to kibana-page-template-header, allowing it to be overridden by consumers if they want something specific. But I haven't yet updated instances.
There was a problem hiding this comment.
Sounds good. ES UI can make the change to our apps and tests in subsequent PRs.
| import { AppMountParameters, ChromeBreadcrumb, ScopedHistory } from 'kibana/public'; | ||
| import { i18n } from '@kbn/i18n'; | ||
| import { ManagementAppWrapper } from '../management_app_wrapper'; | ||
| import { ManagementLandingPage } from '../landing'; |
There was a problem hiding this comment.
Nit: I've observed a general pattern in our other plugins of naming component files the same as the component they export. In this case, it would be management_landing_page.tsx. This convention makes it really easy to hunt down the source when you know the name of the component from reading the consuming code.
There was a problem hiding this comment.
This was already existing, I'm fine with renaming though.
| <ManagementLandingPage | ||
| version={dependencies.kibanaVersion} | ||
| setBreadcrumbs={setBreadcrumbs} | ||
| managementPageLayout={({ children, ...rest }) => ( |
There was a problem hiding this comment.
What's the intention behind this render prop? I'm trying to weigh the benefits against the cost of the added indirection. It looks like the primary benefit of render props is to support code reuse, but I don't see ManagementLandingPage being used anywhere else.
It seems to me like the code would be clearer if ManagementLandingPage imported KibanaPageTemplate, and expected solution, selectedId, and so on to be passed in as props, but I'm probably missing something.
There was a problem hiding this comment.
This is the same way it's handled above with <ManagementAppWrapper /> and my guess is that it's so that each page doesn't have to remember to accept the entire solutionNav configuration.
I just update this portion a bit to resolve conflicts from a previous PR/
There was a problem hiding this comment.
I'm with @cjcenizal on this, including the 'I might be missing something' part. @cchaos seems uncertain so perhaps @myasonik knows.
There was a problem hiding this comment.
@cchaos Tested locally, the changes to Index Management look good! I had a couple suggestions for the broader change.
Looks like the landing page content is sitting a little low. Does it look low to you, too? Also, there's a line at the top of the page that looks odd. Not sure if that's intentional.
This bit is outside the scope of this change -- just wanted to suggest adding some padding to the bottom of the side nav so that when you scroll, you don't have the last few items uncomfortably close to the bottom of the screen. This gets even more uncomfortable when you hover over the last item in Chrome and the little preview of the link's URI shows up in the bottom left corner of the browser. Here's a before and after.
|
I'm working through this right now in ILM, and as I'm passing |
|
I'm not entirely confident about how to solve this, but I believe we can enable my suggestion above by tweaking the way Currently, the logic in that file iterates over all of sections and their apps, renders a I think the solution is to refactor this logic so that the solution navigation is rendered outside of the routes. Basically, we want to see a single instance of the solution navigation that gets rendered only once, separately from the routes. I'm not sure if this is possible given the current dependency upon or design of An immediate solution might be to break |
|
I'll look more closely at the render-prop thread tomorrow but to continue the conversation, if I recall correctly, it was just "the way it worked" to change as little as possible about how the whole system works. Which gets me into all of what you're suggesting @cjcenizal: If we think the architecture isn't stable enough to ship these changes on top of and we need to fix it to move forward, we certainly can. But I just want that to be a very explicit decision and to recognize that's not the ultimate goal here. |
|
Thanks @myasonik, I agree. I'm not interested in pursuing changes unless they help us ship these enhancements to users. After trying to consume this in ILM, I think we do need to pause and reconsider these changes. I found it time-consuming and awkward to use this solution inside of a Management plugin, and there's a real UX problem (flicker-on-navigation, see GIF below) that I believe this architecture brings with it. I think we should address these issues now before we move forward. If we punt on them, then we'll have to go back through our plugins and redo a lot of work later.
|
|
Unfortunately we cannot break apart the KibanaPageTemplate. This component, which just wraps EuiPageTemplate, is the crux of this entire effort. It handles all the layout situations in one consistent way. We've seen how forcing consumers to use individual components breaks down that consistency because no 2 plugins do it the same. If it is helpful, here is a PR that @weltenwort worked on for Observability to create a very similar structure. #99380 |
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / jest / Jest Tests.src/plugins/advanced_settings/public/management_app.AdvancedSettings should render specific setting if given setting keyStandard OutStack TraceKibana Pipeline / jest / Jest Tests.src/plugins/advanced_settings/public/management_app.AdvancedSettings should render read-only when saving is disabledStandard OutStack TraceKibana Pipeline / jest / Jest Tests.src/plugins/advanced_settings/public/management_app.AdvancedSettings should render unfiltered with query parsing errorStandard OutStack Traceand 18 more failures, only showing the first 3. Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
WIP
Checklist
Delete any items that are not applicable to this PR.
For maintainers