add per space configuration to custom header banner #94449

pgayvallet · 2021-03-11T15:51:00Z

Summary

Phase 2 of #17298

Allow per-space configuration of the header banner

Global versus banner configuration

The global configuration remains unchanged (via the kibana.yml configuration file)

The per-space configuration is done using the advanced settings management section, by editing the banners: settings.

The config retrieval logic is the following:

For unauthenticated pages:

Use the global configuration if present, else disable the banner.

For authenticated page:

Use the space configuration (uiSettings) if present, else use the global configuration (kibana.yml), else disable the banner.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Release note

It is now possible to add per-space configuration of the header banner from the banners plugin.

…phase-2

pgayvallet

Self-review

pgayvallet · 2021-03-17T08:32:40Z

src/plugins/advanced_settings/public/management_app/components/field/field.tsx

@@ -326,6 +326,7 @@ export class Field extends PureComponent<FieldProps> {
          <div data-test-subj={`advancedSetting-editField-${name}`}>
            <EuiCodeEditor
              {...a11yProps}
+              name={`advancedSetting-editField-${name}-editor`}


This was required for the addition of SettingsPage.setAdvancedSettingsTextArea (see test/functional/page_objects/settings_page.ts)

pgayvallet · 2021-03-17T08:42:15Z

x-pack/plugins/banners/public/plugin.tsx

-    if (this.config.placement !== 'disabled') {
-      getBannerInfo(http).then(
-        ({ allowed, banner }) => {
-          if (allowed) {
-            chrome.setHeaderBanner({
-              content: toMountPoint(<Banner bannerConfig={banner} />),
-            });
-          }
-        },
-        () => {
-          chrome.setHeaderBanner(undefined);
+    getBannerInfo(http).then(


Now that we got uiSettings-based per-space configuration, I had to remove the optimisation that was checking the configuration to see if the global banner was disabled, as it could be enabled for the current space.

pgayvallet · 2021-03-17T09:04:59Z

x-pack/plugins/banners/server/ui_settings.ts

+    'banners:placement': {
+      name: i18n.translate('xpack.banners.settings.placement.title', {
+        defaultMessage: 'Banner placement',
+      }),
+      category: ['banner'],
+      order: 1,
+      type: 'select',
+      value: config.placement,


The trick here is that I'm using the kibana.yml configuration value as the default value when registering the uiSettings (thanks to the recently added synchronous context.config.get API)

The main upside is a better user experience, as the value displayed as the default one in the advanced settings app is the 'real' default value that would be effectively used. It's also easier to retrieve the computed config as we only need to query the uiSettings API.

elasticmachine · 2021-03-17T11:26:44Z

Pinging @elastic/kibana-core (Team:Core)

afharo

Code review only: LGTM!

afharo · 2021-03-17T13:25:07Z

x-pack/plugins/banners/server/routes/info.ts

+import { IUiSettingsClient } from 'kibana/server';
 import { ILicense } from '../../../licensing/server';
-import { BannerInfoResponse, BannerConfiguration } from '../../common';
+import { BannerInfoResponse, BannerConfiguration, BannerPlacement } from '../../common';
 import { BannersRouter } from '../types';


Super-NIT: import type

afharo · 2021-03-17T13:36:07Z

x-pack/plugins/banners/public/plugin.tsx

@@ -9,35 +9,28 @@ import React from 'react';
 import { CoreSetup, CoreStart, Plugin, PluginInitializerContext } from 'src/core/public';


Super-NIT: import type

flash1293

kibana app changes LGTM, code review only

alexfrancoeur · 2021-03-17T21:28:55Z

Woo! Great work @pgayvallet . Some feedback and comments below

We should probably engage with @ryankeairns to provide some default colors here that aren't brown. Maybe we can just borrow from EUI templates?

I saved a banner and received no notification that it couldn't work due to the license check. I know we discussed adding static text here and it's infinitely more complicated (#94668) to hide / disable the setting or disabling the save button, but I'm going to ask this question any way. Could we provide a toast notification if you save and don't have the appropriate license? It makes for a confusing experience, so anything we can provide here would be great.

Upon starting a trial after the fact, the banner is there as expected.

General banner related question, should we provide an option during configuration (and kibana.yml config) to indicate that you may want any URLs to open in a new tab? Similar to our markdown visualization. I imagine most folks will want that if they add a URL.

I didn't remove the per space banner and added a global config. A refresh persists the old space banner, but if I clear cache and log back in, I'm presented with the new global banner. Two quick things.

Ideally, we'd be able to disable or hide the space config here, but I imagine this is a limitation of uiSettings as well. Do we have any control here?
If the banner is cached, what's the turnaround time to see that updated?

Per space switching is looking great

Overall, this looks good to me. I think we'll need to pull in @gchaps or someone on her team to help with text. @gchaps for context, we cannot perform a license check for any advanced settings today. Custom banners is a feature that is Gold+ only. So in the short term, we'll need to provide some visibile warning text in the Banner card on the advanced settings page. Is there any chance you might be able to help with the content?

pgayvallet · 2021-03-18T07:39:55Z

@alexfrancoeur:

We should probably engage with @ryankeairns to provide some default colors here that aren't brown. Maybe we can just borrow from EUI templates?

It was actually @ryankeairns who provided these default values 😄 . But changing them would be trivial if we want to.

I know we discussed adding static text here and it's infinitely more complicated (#94668) to hide / disable the setting or disabling the save button, but I'm going to ask this question any way. Could we provide a toast notification if you save and don't have the appropriate license? It makes for a confusing experience, so anything we can provide here would be great.

You're going to hate the simplicity of the advancedSettings implementation as much as I started to do, but unfortunately this is not going to be possible without changes to the API. There is currently no 'hook' on the client-side to add such logic, and the settings definition are directly and exclusively retrieved from the server...

I know we discussed adding static text

We talked about it, but in the end I didn't add the 'this setting requires a gold of higher license to be used' description. Do you want me to, given that it's the only think we can really do without any ui/advanced settings improvement?

General banner related question, should we provide an option during configuration (and kibana.yml config) to indicate that you may want any URLs to open in a new tab? Similar to our markdown visualization

The markdown component we're using has a openLinksInNewTab property, so it's technically possible.

Now, do we want to add a configuration option for this, or should we just open all links in new tabs. I feel like the second option would be good enough? I don't see any use case where the user may want to open the link in the current Kibana tab, wdyt?

I didn't remove the per space banner and added a global config. A refresh persists the old space banner, but if I clear cache and log back in, I'm presented with the new global banner. Two quick things.

Hum, can I get more details here? Theoretically, if you initially have a space config and then add a global config after, the space config should still be used when accessing its associated space. Do you mean this wasn't the case?

gchaps · 2021-03-26T15:26:37Z

Using "top" and "disabled" sounds much better to me. We could then put "top" in the description:

Banner placement
Display a top banner for this space, above the Elastic header.

alexfrancoeur · 2021-03-26T17:44:41Z

I'd wish, but no.

🤦 , worth a shot!

Maybe we should just name that placement option top?

I'd be fine with that, would we assume backwards compatibility in for 7.x with header?

pgayvallet · 2021-03-29T06:55:25Z

I'd be fine with that, would we assume backwards compatibility in for 7.x with header?

I'll add a config deprecation for that to log a warning and replace the value used

…phase-2

pgayvallet · 2021-03-29T07:42:32Z

@alexfrancoeur @gchaps Ok, I think we're getting there.

The banner section of the advanced settings:

And the deprecation message if we used was currently using the header value:

I also created #95627 which would unblock adding a description to our categories/sections.

PTAL

alexfrancoeur · 2021-03-29T14:52:17Z

Looking good @pgayvallet !

gchaps · 2021-03-29T16:02:21Z

UI copy LGTM too!

docs/settings/banners-settings.asciidoc

gchaps · 2021-03-29T16:36:49Z

docs/settings/banners-settings.asciidoc

@@ -16,7 +16,7 @@ You can configure the `xpack.banners` settings in your `kibana.yml` file.
 |===

 | `xpack.banners.placement`
-| Set to `header` to enable the header banner. Defaults to `disabled`.


You'll also need to add documentation about the Banner settings to the Advanced Settings page.

Thanks. WDYT about 14c33b6?

…phase-2

pgayvallet · 2021-03-29T20:21:11Z

retest

gchaps · 2021-03-29T20:42:16Z

docs/management/advanced-options.asciidoc

+
+[horizontal]
+[[banners-placement]]`banners:placement`::
+Set to `Top` to display a banner above the Elastic header. Defaults to the value of 


Suggested change

Set to `Top` to display a banner above the Elastic header. Defaults to the value of

Set to `Top` to display a banner above the Elastic header for this space. Defaults to the value of

gchaps · 2021-03-29T20:44:03Z

docs/management/advanced-options.asciidoc

+[float]
+[[kibana-banners-settings]]
+==== Banners
+


Suggest adding: Banners are a subscription feature.

…phase-2

kibanamachine · 2021-03-31T08:23:20Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`advancedSettings`	913.5KB	913.5KB	+48.0B

Page load bundle

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

id	before	after	diff
`banners`	11.9KB	11.7KB	-181.0B

History

💚 Build #116077 succeeded ceea04a
💔 Build #116053 failed 0c8b881
💔 Build #115939 failed 14c33b6
💔 Build #115904 failed 14c33b6
💚 Build #115713 succeeded 2ac2bea

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

* restore the banners ui settings * fix banner init logic * fix unit tests * update telemetry schema * add basic server-side plugin tests * add FTR tests for banners plugin * use keyword for sensitive setting * update snapshots * setting name consistency with configuration properties * fix setting names in telemetry files * open banner links in new tab * add config.disableSpaceBanners property * fix types * add descriptions to banner settings * change label and value header->top * finishing header->top replacement * doc nits * add banners section to advanced options doc * feedback on advanced options doc * adapt deprecation to new format # Conflicts: # src/plugins/kibana_usage_collection/server/collectors/management/schema.ts # src/plugins/kibana_usage_collection/server/collectors/management/types.ts # src/plugins/telemetry/schema/oss_plugins.json

alexfrancoeur · 2021-03-31T10:53:48Z

👏👏👏👏

* restore the banners ui settings * fix banner init logic * fix unit tests * update telemetry schema * add basic server-side plugin tests * add FTR tests for banners plugin * use keyword for sensitive setting * update snapshots * setting name consistency with configuration properties * fix setting names in telemetry files * open banner links in new tab * add config.disableSpaceBanners property * fix types * add descriptions to banner settings * change label and value header->top * finishing header->top replacement * doc nits * add banners section to advanced options doc * feedback on advanced options doc * adapt deprecation to new format # Conflicts: # src/plugins/kibana_usage_collection/server/collectors/management/schema.ts # src/plugins/kibana_usage_collection/server/collectors/management/types.ts # src/plugins/telemetry/schema/oss_plugins.json Co-authored-by: Kibana Machine <[email protected]>

pgayvallet added 2 commits March 11, 2021 16:49

restore the banners ui settings

7faf18e

fix banner init logic

14739df

pgayvallet mentioned this pull request Mar 11, 2021

Customizable Header and Footer Banners #17298

Open

4 tasks

pgayvallet added 7 commits March 16, 2021 09:25

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

50b4dc1

…phase-2

fix unit tests

eedff07

update telemetry schema

4afec76

add basic server-side plugin tests

28b08a5

add FTR tests for banners plugin

559509f

use keyword for sensitive setting

42ad72e

update snapshots

574f8e4

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 labels Mar 16, 2021

pgayvallet added 2 commits March 17, 2021 08:19

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

0c5172f

…phase-2

setting name consistency with configuration properties

9313585

pgayvallet changed the title ~~Customizable header banner - phase 2~~ add per space configuration to custom header banner Mar 17, 2021

fix setting names in telemetry files

fda3ffc

pgayvallet commented Mar 17, 2021

View reviewed changes

pgayvallet added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Mar 17, 2021

pgayvallet marked this pull request as ready for review March 17, 2021 11:26

pgayvallet requested a review from a team as a code owner March 17, 2021 11:26

pgayvallet requested a review from a team March 17, 2021 11:26

pgayvallet requested a review from a team as a code owner March 17, 2021 11:26

afharo approved these changes Mar 17, 2021

View reviewed changes

flash1293 approved these changes Mar 17, 2021

View reviewed changes

pgayvallet added 2 commits March 29, 2021 09:09

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

4b6a76e

…phase-2

change label and value header->top

0570ea1

finishing header->top replacement

2ac2bea

gchaps reviewed Mar 29, 2021

View reviewed changes

pgayvallet added 3 commits March 29, 2021 19:56

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

2ce561a

…phase-2

doc nits

f43168b

add banners section to advanced options doc

14c33b6

pgayvallet requested a review from gchaps March 29, 2021 18:15

pgayvallet mentioned this pull request Mar 29, 2021

[Advanced Settings] hide unknown uiSettings from the UI #94876

Closed

gchaps reviewed Mar 29, 2021

View reviewed changes

pgayvallet added 5 commits March 30, 2021 09:03

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

3f90c42

…phase-2

feedback on advanced options doc

0c8b881

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

ceea04a

…phase-2

Merge remote-tracking branch 'upstream/master' into kbn-17298-banner-…

07206ea

…phase-2

adapt deprecation to new format

f39965b

pgayvallet merged commit ddac0e9 into elastic:master Mar 31, 2021

pgayvallet mentioned this pull request Mar 31, 2021

[7.x] add per space configuration to custom header banner (#94449) #95897

Merged

LeeDr mentioned this pull request Jul 8, 2022

[Archive Migration] x-pack-banners/multispace #135783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add per space configuration to custom header banner #94449

add per space configuration to custom header banner #94449

pgayvallet commented Mar 11, 2021 •

edited

Loading

pgayvallet left a comment

pgayvallet Mar 17, 2021

pgayvallet Mar 17, 2021

pgayvallet Mar 17, 2021

elasticmachine commented Mar 17, 2021

afharo left a comment

afharo Mar 17, 2021

afharo Mar 17, 2021

flash1293 left a comment

alexfrancoeur commented Mar 17, 2021 •

edited

Loading

pgayvallet commented Mar 18, 2021

gchaps commented Mar 26, 2021 •

edited

Loading

alexfrancoeur commented Mar 26, 2021

pgayvallet commented Mar 29, 2021 •

edited

Loading

pgayvallet commented Mar 29, 2021

alexfrancoeur commented Mar 29, 2021

gchaps commented Mar 29, 2021 •

edited

Loading

gchaps Mar 29, 2021

pgayvallet Mar 29, 2021

pgayvallet commented Mar 29, 2021

gchaps Mar 29, 2021

gchaps Mar 29, 2021

kibanamachine commented Mar 31, 2021

alexfrancoeur commented Mar 31, 2021

		@@ -9,35 +9,28 @@ import React from 'react';
		import { CoreSetup, CoreStart, Plugin, PluginInitializerContext } from 'src/core/public';

	Set to `Top` to display a banner above the Elastic header. Defaults to the value of
	Set to `Top` to display a banner above the Elastic header for this space. Defaults to the value of

add per space configuration to custom header banner #94449

add per space configuration to custom header banner #94449

Conversation

pgayvallet commented Mar 11, 2021 • edited Loading

Summary

Global versus banner configuration

Checklist

Release note

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 17, 2021

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

alexfrancoeur commented Mar 17, 2021 • edited Loading

pgayvallet commented Mar 18, 2021

gchaps commented Mar 26, 2021 • edited Loading

alexfrancoeur commented Mar 26, 2021

pgayvallet commented Mar 29, 2021 • edited Loading

pgayvallet commented Mar 29, 2021

alexfrancoeur commented Mar 29, 2021

gchaps commented Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 31, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

alexfrancoeur commented Mar 31, 2021

pgayvallet commented Mar 11, 2021 •

edited

Loading

alexfrancoeur commented Mar 17, 2021 •

edited

Loading

gchaps commented Mar 26, 2021 •

edited

Loading

pgayvallet commented Mar 29, 2021 •

edited

Loading

gchaps commented Mar 29, 2021 •

edited

Loading