Skip to content
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

Merged
merged 13 commits into from
Jun 8, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 3, 2021

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 actual template 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

Empty

<EuiPageContent verticalPosition="center" horizontalPosition="center" color="subdued">
  <EuiEmptyPrompt iconType="" title={<h2></h2>} body={<p></p>} actions={[]} />
</EuiPageContent>

Example commit: bbb74b7

Error states

Error

<EuiPageContent verticalPosition="center" horizontalPosition="center" color="danger">
  <EuiEmptyPrompt iconType="" title={<h2></h2>} body={<p></p>} actions={[]} />
</EuiPageContent>

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 a l (large) size spacer between it and the content.

Page header

<EuiPageHeader
  pageTitle=""
  bottomBorder
  rightSideItems={[
    <EuiButton fill iconType="iconInCircleFilled">
      Create ___
    </EuiButton>,
  ]}
  tabs={[]}
/>

<EuiSpacer size="l" />

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 with hasBorder={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.

Split panels

Example commit: bc3b85b

Checklist

Delete any items that are not applicable to this PR.

@cchaos
Copy link
Contributor Author

cchaos commented Jun 7, 2021

@elasticmachine merge upstream

@cchaos cchaos marked this pull request as ready for review June 7, 2021 19:08
@cchaos cchaos requested a review from a team June 7, 2021 19:08
@cchaos cchaos requested review from a team as code owners June 7, 2021 19:08
@cchaos
Copy link
Contributor Author

cchaos commented Jun 7, 2021

This PR is ready for review, though I'm just double-checking the axe stuff locally at the same time.

@cchaos cchaos added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels Jun 7, 2021
Copy link
Contributor

@cjcenizal cjcenizal left a 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!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
management 43 34 -9

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
advancedSettings 912.7KB 913.0KB +288.0B
indexManagement 1.3MB 1.3MB -708.0B
management 20.0KB 17.0KB -3.1KB
security 767.0KB 766.7KB -278.0B
total -3.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 408.1KB 408.6KB +488.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@myasonik myasonik left a 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}>
Copy link
Contributor

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?

Suggested change
<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."
/>}>

Copy link
Member

@legrego legrego Jun 8, 2021

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:

Suggested change
<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."
/>}>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @myasonik & @legrego , Yeah I'd like to push that decision down to @legrego's conversion. This one I want to try to keep to the basic setup. Changing those links is a slippery slope since they're all over management not just this one.

Copy link
Member

@legrego legrego left a 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}>
Copy link
Member

@legrego legrego Jun 8, 2021

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:

Suggested change
<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."
/>}>

@mbondyra
Copy link
Contributor

mbondyra commented Jun 8, 2021

When running this PR and going to http://localhost:5601/vzy/app/management/insightsAndAlerting/jobsListLink, I get an error in console
Screenshot 2021-06-08 at 16 47 10

Copy link
Contributor

@mbondyra mbondyra left a 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.

@cchaos
Copy link
Contributor Author

cchaos commented Jun 8, 2021

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.

@cchaos cchaos merged commit 51e2da9 into elastic:master Jun 8, 2021
cchaos added a commit to cchaos/kibana that referenced this pull request Jun 8, 2021
…stic#101335)

* Just replace the old wrapper and uses the `solutionNav` prop
* Examples of: Empty Page, Error state, Page Header, and Split Panel
@mbondyra
Copy link
Contributor

mbondyra commented Jun 8, 2021

Thanks @cchaos, I've created an issue: #101621

cchaos added a commit that referenced this pull request Jun 8, 2021
…1335) (#101617)

* Just replace the old wrapper and uses the `solutionNav` prop
* Examples of: Empty Page, Error state, Page Header, and Split Panel
@cchaos cchaos deleted the pageLayouts/stack_management/setup branch March 11, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants