-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Stack Management] Converted to use KibanaPageTemplate (sort of) #101335
[Stack Management] Converted to use KibanaPageTemplate (sort of) #101335
Conversation
@elasticmachine merge upstream |
This PR is ready for review, though I'm just double-checking the axe stuff locally at the same time. |
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.
Changes to the Index Management header look good!
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Tested in browser and code looks good!
defaultMessage="Role mappings define which roles are assigned to users from an external identity provider. {learnMoreLink}" | ||
values={{ | ||
learnMoreLink: ( | ||
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true}> |
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: The link here is super vague ("Learn more"), we can give it a better aria-label
Maybe something like this?
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true}> | |
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true} aria-label={<FormattedMessage | |
id="xpack.security.management.roleMappings.learnMoreLinkAriaLabel" | |
defaultMessage="Learn more about Role mappings." | |
/>}> |
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'll nit @myasonik's nit and suggest a slight casing change. I'm happy to make this in a followup when I convert the rest of the pages if that's easier too:
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true}> | |
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true} aria-label={<FormattedMessage | |
id="xpack.security.management.roleMappings.learnMoreLinkAriaLabel" | |
defaultMessage="Learn more about role mappings." | |
/>}> |
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.
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.
Security changes LGTM - thanks for using some of our screens as an example!
defaultMessage="Role mappings define which roles are assigned to users from an external identity provider. {learnMoreLink}" | ||
values={{ | ||
learnMoreLink: ( | ||
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true}> |
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'll nit @myasonik's nit and suggest a slight casing change. I'm happy to make this in a followup when I convert the rest of the pages if that's easier too:
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true}> | |
<EuiLink href={this.props.docLinks.links.security.mappingRoles} external={true} aria-label={<FormattedMessage | |
id="xpack.security.management.roleMappings.learnMoreLinkAriaLabel" | |
defaultMessage="Learn more about role mappings." | |
/>}> |
When running this PR and going to http://localhost:5601/vzy/app/management/insightsAndAlerting/jobsListLink, I get an error in console |
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.
The console error I mentioned doesn't come from this PR - overall KibanaApp changes look good! tested on chrome.
Thanks @mbondyra for double checking on that console error!! I'll pass that along to the Arch team to see if they know the cause. |
…stic#101335) * Just replace the old wrapper and uses the `solutionNav` prop * Examples of: Empty Page, Error state, Page Header, and Split Panel
Replaces #100085 and supports [META] 7.14 Solution Nav implementation #98359. This is not a feature branch.
The background
KibanaPageTemplate wraps EuiPageTemplate which contains both the side bar and page content and provides several "templates". Unfortunately, Stack Management is setup in a way that is plugin-agnostic until that plugin loads, which causes some complications when trying to use a single component to render both the side (solution) nav and the page contents.
In #100837, we attempted to push the rendering of the template component down to the Router switch mechanism. This forced plugins to reuse a ManagmentPageTemplate (yet another wrapper), but allowed usage of all the template props for consistency and ease. However, this way caused flashes of content while the plugin was loading because it now was also in charge of rendering the side nav.
The solution
The easiest solution that gets us to a point where we can update Stack Management in 7.14 alongside the other solutions, was to keep the solution nav being rendered where it was before by wrapping the router, in
ManagementApp
, with KibanaPageTemplate which gets us the right layout for most pages in Stack Management.Unfortunately, we still can't hoist template props up to this component, so any contents within the plugins/pages, will still have to be manually created. This means using the component EuiPageHeader directly instead of simply passing
KibanaPageTemplat.pageHeader={}
. It also means not being able to change the actualtemplate
type.In this PR I've done the setup for all of Stack Management which will cause most pages to look like a panel within the page.
Examples
There will be a meta ticket setup to help push along the fixes for all these pages, but in this PR I've done some example conversions that are, hopefully, copy-pasteable.
Empty states
Example commit: bbb74b7
Error states
Example commit: 56cf855
If you are having trouble centering your content ensure that any blank divs around your content have the
APP_WRAPPER_CLASS
.Normal page with page header
To be sure the page header renders correctly always add
bottomBorder
and add al
(large) size spacer between it and the content.Example commit: ebc33a2
Panels & Split panels
Be sure that any internal EuiPanel doesn't contain a shadow. You can turn off shadows with
hasShadow={false}
or use borders instead withhasBorder={true}
.EuiSplitPanel is a new component that can be used in some of the unique patterns we see throughout management. Mainly the panels that have headings like in Advanced Settings.
Example commit: bc3b85b
Checklist
Delete any items that are not applicable to this PR.