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

Implement custom global header banner #87438

Merged
merged 36 commits into from
Feb 11, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jan 6, 2021

Summary

This PR Implements the phase 1 of #17298. By adding a new banners plugin that allows users to define a static banner appearing on all page of Kibana.

Screenshots

Login

Screenshot 2021-02-05 at 16 13 27

Stretch to height pages

Screenshot 2021-02-05 at 16 13 14

Scrollable pages

Screenshot 2021-02-05 at 16 13 00

Checklist

Technical implementation

During the discussions in #17298, we came to the agreement to use uiSettings as the underlying implementation of the banner's configuration.

However, I missed two important facts at this time:

  • user-overrides (TIL: even ones done via kibana.yml, I thought it was only the ones done via the UI), are not exposed on unauthenticated pages.
  • when a setting is overridden in kibana.yml, it will always be used. You can't do per-space overrides in that case. Meaning that a uiSettings-only implementation would not have been possible when we would be implementing stage 2.

Given that, I decided to go with a more classic plugin-configuration-based approach for phase1, as we don't need any way to set the global banner configuration via the UI.

Phase 2 will use uiSettings for the per-space banner overrides, which will also allow, with some logic in the middle, to fallback to the global banner configuration, which was not possible with uiSettings.

Also, xpack.banners.placement: 'header' is just a better end user experience than uiSettings.overrides.banner:placement: 'header' when it comes to configuring the banner imho.

Design implementation

There were 3 main tasks here:

  • Adapt the styles of the Kibana header when the banner is enabled to add an appropriate top offset. This is done in src/core/public/chrome/ui/header/_banner.scss
  • Adapt the usages of euiHeaderAffordForFixed to add the banner's height to the body/container offset when the banner is present. Done (mostly) in src/core/public/rendering/_base.scss
  • Adapt all the hardcoded height: calc(100vh - #{$headerHeightOffset}); in various scss files to use a new banner-aware mixin instead.

Release Note

The new banners plugin allows to display a static banner at the top of every page of Kibana. The plugin can be configured via the kibana.yml config file

xpack.banners:
  placement: 'header'
  textContent: 'Production environment - Proceed with **special** levels of caution'
  textColor: '#FF0000'
  backgroundColor: '#CC2211'

This is a gold+ feature, and is disabled on lower license levels.

@pgayvallet pgayvallet changed the title Implement custom footer and header banners Implement custom global header banner Feb 5, 2021
@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.12.0 release_note:feature Makes this part of the condensed release notes labels Feb 5, 2021
@pgayvallet
Copy link
Contributor Author

Screenshot 2021-02-09 at 12 11 23

Screenshot 2021-02-09 at 12 07 57

@alexfrancoeur
Copy link

No, waiting on input here. Realistically if we want to have a chance to see this lands in 7.12, the smaller default is the only option, but I need to know the expected banner height and text-size.

++ on a smaller default without any config for 7.12. @ryankeairns any thoughts / suggestions?

Totally forget to implement that. There should be a markdown component already existing somewhere, so it may be trivial.

Woah, awesome! Thanks Pierre

I'll let him answer, but I guess it would be a yes. However I see that in fullscreen mode, the banner is overlapping on the content. I guess I need to fix that.

++ I agree that it should probably be there, thanks for fixing the overlapping content.

Can do that. Not sure how to display the warning though? I don't think we can do better than in the server logs at startup.

Yeah, server logs feel like the only option here. I wonder if it's worth it though. Maybe we just add a recommended character limit in the docs?

I honestly have no idea what reporting is altering when navigating with their headless. It could even be a distinct media target. This one may be hard if we want to display the banner here.

Yeah, let's not worry about this now and see if folks ask for this later.

@mbarretta
Copy link

No, waiting on input here. Realistically if we want to have a chance to see this lands in 7.12, the smaller default is the only option, but I need to know the expected banner height and text-size.

++ on a smaller default without any config for 7.12. @ryankeairns any thoughts / suggestions?

Been slow to respond, but I'm also happier with the smaller default. I just mentioned in this comment that we've been giving people a plugin with the height set to 24px. Not suggesting this must be the value, but generally smaller is better for my users' use case

Totally forget to implement that. There should be a markdown component already existing somewhere, so it may be trivial.

Woah, awesome! Thanks Pierre

++ nice!

I'll let him answer, but I guess it would be a yes. However I see that in fullscreen mode, the banner is overlapping on the content. I guess I need to fix that.

++ I agree that it should probably be there, thanks for fixing the overlapping content.

Yes, agree that it should be present when in full screen mode. If LOE is high, and implementing it risks kicking this out of the next release, I'd be ok with it being marked as a known issue.

Can do that. Not sure how to display the warning though? I don't think we can do better than in the server logs at startup.

Yeah, server logs feel like the only option here. I wonder if it's worth it though. Maybe we just add a recommended character limit in the docs?

Considering much depends on the client's screen size, a general note in the docs about the wrapping behavior and implications is probably enough.

...maybe even white-space: nowrap?

I honestly have no idea what reporting is altering when navigating with their headless. It could even be a distinct media target. This one may be hard if we want to display the banner here.

Yeah, let's not worry about this now and see if folks ask for this later.

++
This is the same as full screen mode. The banner should be displayed everywhere, even on printed reports, but think we can push forward, mark it as a known issue, and tackle in the future.

@ryankeairns
Copy link
Contributor

Replied about the height here: #87438 (comment)
The smaller default is all good by me. Let's compromise and make it 32px, so $euiSizeXL

@VijayDoshi
Copy link

This is great, many customers will be happy!

@pgayvallet
Copy link
Contributor Author

Changed the banner height to $euiSizeXL. Just wondering if the text doesn't look 'too fat' now? It's currently 16px, is that alright or should we decrease it a little?

Screenshot 2021-02-10 at 08 05 25

Only waiting for a review from @elastic/kibana-design now

@mbarretta
Copy link

It's currently 16px, is that alright or should we decrease it a little?

Looks good to me, but I'm not renowned for my aesthetic eye

@ryankeairns
Copy link
Contributor

ryankeairns commented Feb 10, 2021

Changed the banner height to $euiSizeXL. Just wondering if the text doesn't look 'too fat' now? It's currently 16px, is that alright or should we decrease it a little?

Yeah, let's make it a bit smaller. Try $euiFontSizeS (14px).

@ryankeairns ryankeairns self-requested a review February 10, 2021 15:24
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

I ran through the apps in left nav, clicked through the header, opened any flyouts I could find, etc. Everything work as expected although I don't have 100% everything enabled and am using only sample data. I feel comfortable with where things stand and only left a couple of comments.

Great work @pgayvallet !

Comment on lines +3 to +4
/* stylelint-disable-next-line length-zero-no-unit -- need consistent unit to sum them */
@mixin kibanaFullBodyHeight($additionalOffset: 0px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably maneuver around this to avoid the need for the disable line, but then we end up with more 'stuff' for a very minimal gain. I prefer the clarity here of what is implied by the default value, as well.

Comment on lines +15 to +22
.kbnBody--hasHeaderBanner .header__bars {
.header__firstBar {
top: $kbnHeaderBannerHeight;
}
.header__secondBar {
top: $kbnHeaderBannerHeight + $euiHeaderHeightCompensation;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also revisit this once the page layout service is available.

Comment on lines +1 to 8
@import '../../../../../core/public/mixins';

.homWrapper {
@include kibanaFullBodyMinHeight();
background-color: $euiColorEmptyShade;
display: flex;
flex-direction: column;
min-height: calc(100vh - #{$euiHeaderHeightCompensation});
}
Copy link
Contributor

@ryankeairns ryankeairns Feb 10, 2021

Choose a reason for hiding this comment

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

Any thoughts on my prior comment? Overall, a minor item that I wouldn't block merging on.

@alexfrancoeur
Copy link

Dropping a note here. When we merge this we'll want to open an issue in the Cloud repo to whitelist the config.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
advancedSettings 56 57 +1
banners - 18 +18
core 444 446 +2
total +21

Async chunks

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

id before after diff
advancedSettings 919.5KB 920.1KB +625.0B
discover 420.8KB 424.6KB +3.9KB
home 182.9KB 184.9KB +2.1KB
security 724.4KB 724.9KB +560.0B
spaces 41.2KB 41.5KB +268.0B
total +7.3KB

Page load bundle

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

id before after diff
banners - 13.6KB +13.6KB
core 467.9KB 475.6KB +7.7KB
kibanaOverview 45.0KB 46.9KB +1.8KB
maps 139.0KB 140.5KB +1.5KB
painlessLab 22.2KB 24.0KB +1.8KB
searchprofiler 45.7KB 47.4KB +1.7KB
total +28.1KB

History

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

@pgayvallet pgayvallet merged commit 570dcc0 into elastic:master Feb 11, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Feb 11, 2021
* first draft

* update plugin list

* fix tsproject

* update bundle limits file

* remove unused start dep

* adapt imports

* POC of footer banner

* update styles, mostly

* plug banner to uiSettings

* adding some unit tests

* add tests on sort_fields

* cleanup sums in sass mixins

* some self review stuff

* update generated doc

* add tests for color field

* update chrome header test snapshots

* retrieve license info from the server

* switch from uiSettings to plugin config

* update plugin list description

* update default colors

* NIT

* add markdown support

* fix banner overlap in fullscreen mode

* change banner height to 32px

* change banner's font size to 14

* delete unused uiSettings
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 11, 2021
* master: (44 commits)
  [APM] Add experimental support for Data Streams (elastic#89650)
  [Search Session] Control "Kibana / Search Sessions" management section by privileges (elastic#90818)
  [Lens] Median as default function (elastic#90952)
  Implement custom global header banner (elastic#87438)
  [Fleet] Reduce permissions. (elastic#90302)
  Update dependency @elastic/charts to v24.5.1 (elastic#89822)
  [Create index pattern] Can't create single character index without wildcard (elastic#90919)
  [ts/build_ts_refs] add support for --clean flag (elastic#91060)
  Don't clean when running e2e tests (elastic#91057)
  Fixes track_total_hits in the body not having an effect when using search strategy (elastic#91068)
  [Security Solution][Detections] Adds list plugin Saved Objects to Security feature privilege (elastic#90895)
  Removing the code plugin entirely for 8.0 (elastic#77940)
  chore(NA): move the instruction to remove yarn global bazelisk package into the first place on install bazel tools (elastic#91026)
  [jest/ci] remove max-old-space-size override to use 4gb default (elastic#91020)
  [Fleet] Restrict integration changes for managed policies (elastic#90675)
  [CI] Fix auto-backport condditions so that it doesn't trigger for other labels (elastic#91042)
  [DOCS] Uses variable to refer to query profiler (elastic#90976)
  [App Search] Relevance Tuning logic listeners (elastic#89461)
  [Metrics UI] Fix saving/loading saved views from URL (elastic#90216)
  Limit cardinality of transaction.name (elastic#90955)
  ...
pgayvallet added a commit that referenced this pull request Feb 11, 2021
* first draft

* update plugin list

* fix tsproject

* update bundle limits file

* remove unused start dep

* adapt imports

* POC of footer banner

* update styles, mostly

* plug banner to uiSettings

* adding some unit tests

* add tests on sort_fields

* cleanup sums in sass mixins

* some self review stuff

* update generated doc

* add tests for color field

* update chrome header test snapshots

* retrieve license info from the server

* switch from uiSettings to plugin config

* update plugin list description

* update default colors

* NIT

* add markdown support

* fix banner overlap in fullscreen mode

* change banner height to 32px

* change banner's font size to 14

* delete unused uiSettings
@m-adams
Copy link

m-adams commented Mar 5, 2021

Great to see this, looking forward to the settings being whitelisted so we can try it in staging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:feature Makes this part of the condensed release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.