search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.69k stars 8.12k forks source link

Customizable Header and Footer Banners #17298

Open alexfrancoeur opened 6 years ago

alexfrancoeur commented 6 years ago

Some work environments require a constant reminder of the environment that you are in. Ideally, this would be surfaced as a persistent banner with customizable text that cannot be dismissed. The banner(s) should be completely optional and shown in all aspects of Kibana, including the login screen. In order to fulfill this use case, Kibana would need the following:

Also somewhat related (albeit closed) is https://github.com/elastic/kibana/issues/4452

Some notes:

Stages

ryankeairns commented 3 years ago

Let's take an iterative approach here, get something in place that addresses 80% of cases, and plan for future enhancements. Design-wise, I'm confident that we can go with a fixed-height approach that flexibly handles long,. overflowing text. We'd have to take this approach regardless for smaller screens, so let's head that route and Michael or I will provide some design options for handling the long text.

ryankeairns commented 3 years ago

@pgayvallet Your inclinations about the footer approach resonate with me, including the point about letting the nav and flyout go over top... at least for this initial version. Handling that any other way will significantly delay the availability of a solution.

pgayvallet commented 3 years ago

@ryankeairns ok, I will try to implement that approach then, thanks.

alexfrancoeur commented 3 years ago

A bit behind on the thread here, but I think all questions / answers align with my thoughts as well 👍

Regarding telemetry collection, I do think we need to make a white list for the advanced settings page. I saw a thread somewhere where this looked like a low effort, so we should prioritize.

ryankeairns commented 3 years ago

In the spirit of keeping the text visible, accommodating lengthy text, and keeping things simple, here are a couple of mockups that depict the fixed-height approach coupled with a basic horizontal scrolling behavior.

We might also consider an interaction (button or link) that opens a popover or modal, but a) I wonder if that strays from the 'always visible' requirement and b) that might be tricky for plain text supplied via the config file (i.e. where do we cut the text, what goes into popover, etc.). I'm happy to mock that up if people find it valuable.

At 1140px wide, laptop, 217 visible characters

Screen Shot 2021-01-07 at 3 12 41 PM

At 940px wide, smaller window/device, 135 visible characters

Screen Shot 2021-01-07 at 3 13 02 PM

Prototype

Try this rough prototype to get a feel of the scrolling behavior. We could further style the scrollbar with additional EUI styles.

https://codesandbox.io/s/headless-waterfall-ubd98?file=/calloutBanner.css

pgayvallet commented 3 years ago

@ryankeairns @alexfrancoeur @mbarretta Do we want the header banner to be considered as part of the header when we are expanding the navigation menu?

Screenshot 2021-01-11 at 12 42 01

In this screenshot, if we had a top banner, should the left nav expand after the banner (keeping the banner fully visible even when the left nav is expanded), or should the banner be partially hidden on the left, as the page's body?

Remark: in term of implementation, the easiest would be to consider the banner as part of the header, therefor having the left nav NOT overlapping on it when expanded.

mbarretta commented 3 years ago

@pgayvallet Maybe this puts my users on an island, but it needs to be above and below all other content. The header must be above the black Elastic header bar and the footer would go at the absolute bottom.

My crappy mock up demonstrates this since all of the left nav exists between the header and footer.

pgayvallet commented 3 years ago

but it needs to be above and below all other content

So the top banner MUST be on top of the Kibana header, right?

pgayvallet commented 3 years ago

@ryankeairns After a few attempts, I think that this is unfortunately going to be more tedious than expected:

All the 'stretch-to-height' apps seems to manually hardcode the header values too, causing the footer banner to overlap on them:

Screenshot 2021-01-11 at 15 32 50

https://github.com/elastic/kibana/blob/a417690ced52b13b3f1bd5c440ee26fc7eca57c2/src/plugins/discover/public/application/components/discover.scss#L5-L8

Which means that, if we continue in that direction:

Also note that using padding-bottom on the body seems to have no effect, and that we are forced to apply the bottom padding on the nested #kibana-body element instead. However this causes the scrollbar to always be present, even if the content of the page does not overflow the body's height:

Screenshot 2021-01-11 at 15 45 28

Another thing:

To position the left nav (well, all the flyouts) correctly for it to not appear over the bottom banner, top+bottom positioning is not enough, and we will need to use top + bottom + height, meaning that here too, we need a single header+footer mixin and distinct css classes for each situation (header/footer/both/none).

Screenshot 2021-01-11 at 15 50 35

Last but not least: Integrating that into chromeless apps (such as the login page), which are currently just using hiding the whole chrome header is probably going to be another challenge, even if I did not check that specific part yet.

@ryankeairns (cc @joshdover @alexfrancoeur ) The more I progress, the more I think we may need the design team to help us on that one. Would it be realistic to have the design team help us on the banner design integration once I finish the logic behind it? https://github.com/elastic/kibana/pull/87438 should soon be 'technically' ready (displaying the banners depending on the user configuration)

ryankeairns commented 3 years ago

@pgayvallet I agree that we should take a step back and think through the design (UI implementation approach) a bit further. With @mbarretta 's requirement for this to be first in order, we're going to face additional complications as well since this essentially affects the left nav + flyouts across the board. Additionally, the footer is something we'll need to investigate further as well. There are EUI components that display at the bottom of a page that we can probably gain insights from.

All in all, there are many impacts here as you've encountered. I would like to chat with the experts on the EUI team, conjure up possible solutions, and perhaps prototype this out. Given the potential for wide-ranging impact, we'll need to comb through the apps and insure that the layout is not broken.

I'll coordinate with the EUI team today/tomorrow and report back with a plan.

alexfrancoeur commented 3 years ago

Thanks for the support @ryankeairns, we knew this one might be tricky

ryankeairns commented 3 years ago

I've chatted with several folks from the Kibana Design and EUI teams and here is our proposed plan.

TL;DR

Coincidentally, there is an effort under way to provide a page layout service in Kibana that would standardize how plugins supply content within the global 'wrapper'. We're pursing that for several reasons (better consistency, better accessibility, etc.) and we believe that the requirements for these banners can be accommodated as part of this effort.

Getting there

I'll spare all the details for now, but suffice it to say that adding fixed elements to the existing UI (particularly the footer) is tricky in that we'll have to account for the handling of many existing UI elements (flyouts, the global/left nav, bottom bars, pages with independent scrolling, etc.).

In the near term, we could proceed with wiring up the configuration and display the top banner (using the approach described above), but it strikes me as not meeting one of the fundamental requirements - having a banner in the footer. @pgayvallet likely has thoughts on what backend work, if any, he can continue building out, but the UI pieces likely need to wait.

Foregoing the near term, top-banner-only option, the order of events would look like this:

The bad news is that it will take longer; likely a minor for each step. The good news is that the UI component work is under way and the Kibana Design team is standing by to begin work on the service. Also, this seems our best bet at meeting the core requirements (always visible top and bottom banners).

How do folks feel about this approach?

pgayvallet commented 3 years ago

How do folks feel about this approach?

Even if that probably means that it is unlikely to have the feature ready for 7.12, I think it's the correct option here. Trying to hack our way into displaying a static footer would be a nightmare to maintain, and having proper development from the EUI/design teams that we could leverage for the feature seems a way better solution.

In the near term, we could proceed with wiring up the configuration and display the top banner (using the approach described above), but it strikes me as not meeting one of the fundamental requirements - having a banner in the footer

I think nothing is blocking us from doing so, but I agree that imho this will not be good enough given the requirements and that it is probably useless to do that now. @mbarretta can you confirm that?

mbarretta commented 3 years ago

In the near term, we could proceed with wiring up the configuration and display the top banner (using the approach described above), but it strikes me as not meeting one of the fundamental requirements - having a banner in the footer

Correct. This would be a gesture of sorts, showing we're going to meet this requirement, but users would still need custom plugins to achieve accreditations.

alexfrancoeur commented 3 years ago

If we're able to provide a top level banner, with kibana.yml config and space advanced setting support, I think it's progress towards our end goal. This approach allows administrators to identify environments and spaces more visually. I vote we take this incremental step, keep this issue open and continue to prioritize the efforts in EUI until the footer requirement is met. For the top banner in this proposed approach, does it persist or does it go away when you scroll?

pgayvallet commented 3 years ago

For the top banner in this proposed approach, does it persist or does it go away when you scroll?

it would be persistent (same way as the 'Kibana' header)

pgayvallet commented 3 years ago

@mbarretta @alexfrancoeur I looked a little more closely to the interactions between uiSettings and security.

What I learned if that, when a user is not authenticated, we can't retrieve the uiSettings overridden from the UI, as this would mean be performing an ES request, which is not allowed by default as we're not authenticated.

What does that mean? Basically, with the uiSettings approach, we wouldn't be able to display the banner(s) on anonymous pages, which include the login page. That would go against the requirement: shown in all aspects of Kibana, including the login screen

I think I could work around that by exposing a custom endpoint that the banner plugin will use, that would be performing the ES request with the internal user instead of the currently authenticated user.

A few remarks on that workaround:

mbarretta commented 3 years ago

I again can only speak for my users.

For them, there really is no need to have settings overridden by the UI or be different for each Space. If everything is easier by having settings live only in kibana.yml, which wouldn't need any call backs to ES (authenticated or otherwise), then we're happy.

For the broader community, UI overrides and per-Space settings make sense of course, but my users would be happy if the behavior in that case was to fall back to the default yml values.

I'm just surprised that a US gov. specification requires to display something considered sensitive (as we were asked to not collect them in telemetry) on unauthenticated pages?

The banner contents aren't sensitive outside of the network they are on. For example, a banner displayed here on GitHub might say: "You are on the PUBLIC INTERNET version of GitHub: don't commit passwords because that'd be stupid"

Whereas one on a private intranet might say: _"You are on the internal network GMEROCKETSHIP version of GitHub: committing passwords is ok, but also kind of stupid"

legrego commented 3 years ago

I think I could work around that by exposing a custom endpoint that the banner plugin will use, that would be performing the ES request with the internal user instead of the currently authenticated user.

One oddity here is that you'd be displaying the header/footer on the login page, using content configured for a specific space (likely the default space). That could be confusing for some users if they have different banners per space.

pgayvallet commented 3 years ago

For them, there really is no need to have settings overridden by the UI or be different for each Space. If everything is easier by having settings live only in kibana.yml, which wouldn't need any call backs to ES (authenticated or otherwise), then we're happy.

One oddity here is that you'd be displaying the header/footer on the login page, using content configured for a specific space (likely the default space). That could be confusing for some users if they have different banners per space.

In that case, would it be acceptable for everyone, (for the initial implementation at least), to declare that we are only supporting global-scope banners, and not per-space banners (by having the banner ui-settings defined as readonly)?

That way

@alexf @choobinejad @mbarretta wdyt

The banner contents aren't sensitive outside of the network they are on

Thanks @mbarretta for the explanation.

choobinejad commented 3 years ago

This makes sense to me @pgayvallet. We're trying to stay focused on the hard security/compliance requirement of having the banners, and then we can iterate on it later, adding useful features like per-Space messages.

alexfrancoeur commented 3 years ago

I do have one question for the team. Generally, I think that taking a phased approach makes sense here. Starting wide (global) and moving to more granular use cases (space-specific). My concern lies with whether or not we'd be meeting the requirements for accreditation if we are only able to surface the header and not a footer. Will this suffice for our customer and community members that need this requirement?

From participation in this thread earlier, my feelings were that it did not. "Header only" is however, useful for other use cases such as visually differentiating spaces based on environment, team, customer, etc. For the space-specific requirements, I do not believe we need banners on the login page itself. And if it'd be helpful to separate requirements for each use case, I'm happy to open a separate issue. They are very similar, but some key differences. Primarily the login page and administrative expectations.

If "header only" meets the requirements for @mbarretta's customers, then I think it does make sense to prioritize the global kibana.yml approach. If footers are required, we now know that work is a bit further out and it probably worth refocusing our initial implementation on the broader use cases.

ryankeairns commented 3 years ago

In the near term, we could proceed with wiring up the configuration and display the top banner (using the approach described above), but it strikes me as not meeting one of the fundamental requirements - having a banner in the footer

Correct. This would be a gesture of sorts, showing we're going to meet this requirement, but users would still need custom plugins to achieve accreditations.

@alexfrancoeur that question came up earlier in this thread, as well. This was Mike's reply ^

alexfrancoeur commented 3 years ago

++ that was how I remembered it as well. If it's really just a gesture, I feel like we should focus our initial efforts on spaces first approach then. @joshdover @pgayvallet should we discuss during the core sync tomorrow? I'd like to come to a conclusion on the initial phase first if possible. If we're truly blocked by EUI improvements for footer support, then we'll need to wait a bit longer to support @mbarretta and his user base. I think we can all agree that we want to support both requirements at the end of the day.

m-adams commented 3 years ago

@alexfrancoeur for the users I work with who have this requirement, a header would be sufficient for initial accreditation. I agree that I would rather have something header only and global if it meant we could get an initial version faster as the accreditation use case is the most important from my perspective.

alexfrancoeur commented 3 years ago

After a bit more research and a few follow up discussions, I think we've landed on a plan here. There are enough logged ER's for a persistent header (with no mention of footer) that it makes sense to prioritize this approach first. After syncing with @pgayvallet we agree on this phased approach to delivering this feature for multiple use cases.

pgayvallet commented 3 years ago

I'm currently working on phase 2, to allow per-space banner configuration.

The per-space config is going to leverage the ui settings, with fallback to the default/global config when the settings are not defined for this spaces.

I got a POC working in https://github.com/elastic/kibana/pull/94449.

The main issue I'm facing now is on how to hide the banners settings from the advanced settings page when the license level is not gold+.

Currently, when the license level is not gold or higher, the banners feature is disabled, and both the global and per-space configurations are just ignored. However in that situation, the banners uiSettings are still registered, and therefor are displayed in the advanced settings page.

I'm surprised this is the first time we have such license-level dependent settings registration, but AFAIK it is... The problem is, settings registration must be performed synchronously during the setup phase, which mean we just cannot wait for the asynchronous licensing API and the initial license fetch to register the settings.

Ideally, we could just add a licenseLevel field to the UiSettingsParams, allowing to define a minimum license level to use and edit the setting using the API and the UI. Problem is, the uiSettings implementation has no knowledge of the concept of license level in any way.

I see a couple workarounds for that, but nothing I would really proud of implementing to be honest:

Not really a big fan of the approach. Imho we shouldn't need to unregister or disable settings, and have proper APIs to only register them if needed, or to update the setting definition if required.

That's not in core's setup vs start philosophy. Also not sure of all the side effects. May still be the easiest solution though. Would require to change the way we validate the definitions and overrides, as validation is currently performed once during uiSettings start step.

Note that this is more impactful that it seems, as currently the uiSettingsParams value is serializable and directly sent to the client. adding functions attributes will forces to check the whole code base to convert/call the function properties before the serialization.

Note that we had similar problems with saved objects definition and the need to update them dynamically, and we're going to go with a hook API to address the SO problem. But in the case of SO definitions, all the updates were performed before the beginning of the start phase. Our situation here is even worse, as the initial license fetch required to enable or not the settings is performed during licensing start phase.

@elastic/kibana-core what do you think?

mshustov commented 3 years ago

I'm surprised this is the first time we have such license-level dependent settings registration, but AFAIK it is...

uiSettings do not have their own UI, they are rendered via advanced settings plugin, which doesn't know about x-pack concepts of licensing and RBAC. Only plugins might define business rules for licensing and RBAC. It means that either we need to provide an API for plugins to change uiSettings status before rendering them in advanced settings (something similar to the disable method in one of your suggestions) or probably invert the dependency and make plugins responsible for rendering uiSettings (within a UI container on advanced settings page or inside their application). Either option is out of the scope of the current PR. I'd suggest creating another enhancement issue for this.

In the short team, is it a real problem to render the banner section in advanced settings? We do pretty much the same right now. For example, I can create a user without ml permissions, but I still can change ML settings in advanced settings UI.

pgayvallet commented 3 years ago

It means that either we need to provide an API for plugins to change uiSettings status before rendering them in advanced settings (something similar to the disable method in one of your suggestions)

I think just delegating the widget / setting rendering to the plugins will not be sufficient, as we ideally would like to totally disable the setting (e.g the http api or server-side API would also consider the setting as disabled or non-existing). But we probably need to discuss that more in depth, which seems, I agree, outside of the scope of this PR / feature.

In the short team, is it a real problem to render the banner section in advanced settings? We do pretty much the same right now. For example, I can create a user without ml permissions, but I still can change ML settings in advanced settings UI.

That's a valid point.

@alexfrancoeur: Do you think it would be alright to handle the uiSettings/advancedSettings 'permissions' and/or 'visibility' in a follow-up? There isn't any easy workaround to address that in current PR, and deciding on the proper implementation to be generic enough to support our future similar needs (which imho are going to increase in number, given that we'll have more and more features under a minimal license level) may take some time. Trying to solve this in current PR will likely delay the banner phase 2 delivery a lot.

As a temporary (poor man's) measure, I could add a This setting requires a gold or higher license to be used as the description of all banner settings, wdyt?

joshdover commented 3 years ago

+1 on this problem being out of scope for this change.

TBH, the advanced settings UI & service need a general overhaul or 'growing up' in several different ways (required validation, strict schema, dedicated REST APIs, etc.). We can keep tacking on features to the existing service (such as a disable API) but I think it'd be best if we started from scratch here with a v2. IMO this should be considered as part of https://github.com/elastic/kibana/issues/17888 as consider how we want user and space settings to work in the long-term.

As a temporary (poor man's) measure, I could add a This setting requires a gold or higher license to be used as the description of all banner settings, wdyt?

This seems like an ok compromise. Another might be to render our own advanced setting UI similar to how the Telemetry plugin is doing this for the opt-in. I believe one drawback of this approach is lack of i18n and I think the search bar at the top doesn't work. So maybe not a good option for long-term.

alexfrancoeur commented 3 years ago

As a temporary (poor man's) measure, I could add a This setting requires a gold or higher license to be used as the description of all banner settings, wdyt?

👍 I think this is fine for an MVP, but feels like a poor user experience. We'll need to address this problem eventually. Especially as we begin to think through user settings, personalization and customization such as themes / white labeling in the future. If it's low effort to roll our own advanced setting UI similar to telemetry that would allow for this type of license check, it's probably worth exploring to reduce confusion.

I think it'd be best if we started from scratch here with a v2

I agree with this. Regardless if whether or not we take a similar approach to telemetry here, we should track this need and maybe spin up a meta issue to start capturing requirements for (user / space / global) settings v2.

pgayvallet commented 3 years ago

I think this is fine for an MVP, but feels like a poor user experience. We'll need to address this problem eventually.

I definitely agree. This is something we want to fix, and probably more mid term than long term. I really feel we will encounter more and more similar needs for future settings.

Regardless if whether or not we take a similar approach to telemetry here, we should track this need and maybe spin up a meta issue to start capturing requirements for (user / space / global)

The user settings/data architecture is still in progress, and may impact that a lot, but I will create a meta issue and a sub issue for the specific problem we encountered here.

pgayvallet commented 3 years ago

Created https://github.com/elastic/kibana/issues/94668

dudeisbrendan03 commented 3 years ago

Is it likely we'll see this in Elastic 8?

pgayvallet commented 3 years ago

@dudeisbrendan03 The header banner is already available, as a licensed feature, since 7.12

dudeisbrendan03 commented 3 years ago

@dudeisbrendan03 The header banner is already available, as a licensed feature, since 7.12

Oh! Didn't know that, cant seem to find any docs about it anywhere

pgayvallet commented 3 years ago

I think the only reference to the feature we currently have is on the settings configuration page: https://www.elastic.co/guide/en/kibana/master/advanced-options.html#kibana-banners-settings

dudeisbrendan03 commented 3 years ago

Thanks!

dudeisbrendan03 commented 3 years ago

Ahh that wasn't what I thought it was, we're trying to customise the login screen. Thanks anyway

mbarretta commented 3 years ago

@dudeisbrendan03 looking for system use notification like this: https://github.com/elastic/kibana/pull/63563 ?

mbarretta commented 2 years ago

with https://github.com/elastic/kibana/pull/94449, I think phase 2 of this issue was completed. Any insight on plans for phase 3, the global footer?

pgayvallet commented 2 years ago

Any insight on plans for phase 3, the global footer?

https://github.com/elastic/kibana/issues/86528 is still a hard requirement to add support for a global/fixed footer. @elastic/kibana-design, any progress or visibility on that one?

ryankeairns commented 2 years ago

Any insight on plans for phase 3, the global footer?

86528 is still a hard requirement to add support for a global/fixed footer. @elastic/kibana-design, any progress or visibility on that one?

There are no existing plans in place that I'm aware of regarding the next phase. That particular issue is more about a11y improvements to be gained, in parallel. In some ways we've made progress - there are new EUI page layout components, those are wrapped by a Kibana page wrapper (that we're starting to use on empty state pages), but actually wrapping the entire app in such a way to support an always visible banner/footer has not been discussed since the current banners were put in place.

I can bring it up on the design side for more clarity on possible implementation approaches but, as with all things, prioritization on the Eng/Product side is the ultimate factor in getting it moving.

pgayvallet commented 2 years ago

as with all things, prioritization on the Eng/Product side is the ultimate factor in getting it moving.

cc @alexfrancoeur then (not sure if you're still directly owning this, feel free to forward the ping)

mbarretta commented 2 years ago

Thanks for the updates, all. The header helped, but the footer is still an ongoing requirement for a large number of federal users. Are we in the post-8.0 timeframe already?

pgayvallet commented 2 years ago

Are we in the post-8.0 timeframe already

Given @ryankeairns's answer, most definitely yes.

cc @thesmallestduck

1Copenut commented 2 years ago

Adding a bit to this issue. A customer is asking for full HTML in these customer footers so they can add ARIA markup for accessibility. I'm working to bracket the exact use case, but it sounds like this custom footer is being shown/hidden dynamically and needs markup to announce to screen readers / assistive tech that the footer has been added to the page.

cchaos commented 2 years ago

@1Copenut Just a note, but the header/footers have been restricted to certain content because they're also restricted/truncated to a specific height. Is the ARIA markup they were going to implement something that should just be inherent in the banner itself?

1Copenut commented 2 years ago

@cchaos I've asked for more clarification, but my hunch is yes, this might be something inherent to the banner. If that's the case, I'll update my comment and open a ticket in EUI as needed.

1Copenut commented 2 years ago

@cchaos and @ryankeairns Follow on to my previous comments from a week ago. We had a quick demo call with the customer and I was able to get verbal confirmation the customer is wanting full HTML editing capability within the custom banners. Their use case involves creating content (links, text, ARIA) that is custom to their install. EuiMarkdownFormat appears to support GitHub style Markdown only.