[Banner Plugin] UI Implementation Refactor #10125

sidwang42 · 2025-07-11T21:29:55Z

Description

Adds a lightweight header wrapper and shared --global-banner-height / $osdHeaderOffset variables so all headers, body content, flyouts and masks automatically shift downward whenever the Global Banner is mounted. Removes all !important overrides and hard-coded pixel offsets.

Issues Resolved

The concerns raised in #9989 (comment)

Screenshot

Implementation Details

This change avoids global OUI framework CSS overrides by dynamically offsetting headers and page content using a shared layout strategy:

Banner Mounting

The banner is rendered as a fixed element: #pluginGlobalBanner
On mount, it sets a CSS variable:

--global-banner-height

Header Wrapping

A new class .headerGlobalNav--withBanner is conditionally applied when the banner is enabled.
This class modifies header positioning:

.headerGlobalNav--withBanner .expandedHeader {
  top: var(--global-banner-height);
}

.headerGlobalNav--withBanner .primaryHeader:not(.newTopNavHeader) {
  top: calc(var(--global-banner-height) + $headerWidth);
}

Shared Offsets

Body content, flyouts, and overlay masks use shared SCSS variables:

$osdHeaderOffset: calc(#{$euiHeaderHeightCompensation} + var(--global-banner-height, 0px));
$osdHeaderOffsetExpanded: calc((#{$euiHeaderHeightCompensation} * 2) + var(--global-banner-height, 0px));

These values are injected into min-height, padding, and top styles to ensure content shifts automatically when the banner appears.

Fallback Safety

If the banner is not rendered, --global-banner-height defaults to 0px, preserving the existing layout when the banner is disabled.

Testing the changes

Start Dashboards with banner disabled – verify no layout change.
Enable banner plugin (opensearch_dashboards.yml).
Refresh any app page:
- Banner appears at top.
- Expanded Header & Primary Header reposition correctly.
- Flyouts / overlay masks open without overlap.
Resize window, and switch to dark mode – confirm offsets remain correct.

Changelog

feat: add wrapper & CSS-variable offsets to support Global Banner without !important hacks

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update [CHANGELOG.md](./../CHANGELOG.md)
Commits are signed per the DCO using --signoff

codecov · 2025-07-11T21:42:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.70%. Comparing base (01a7a62) to head (06e7911).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10125   +/-   ##
=======================================
  Coverage   59.70%   59.70%           
=======================================
  Files        4031     4031           
  Lines      102246   102246           
  Branches    16139    16139           
=======================================
  Hits        61051    61051           
  Misses      37185    37185           
  Partials     4010     4010

Flag	Coverage Δ
Linux_1	`27.57% <0.00%> (ø)`
Linux_2	`56.58% <100.00%> (ø)`
Linux_3	`38.27% <0.00%> (+<0.01%)`	⬆️
Linux_4	`28.99% <0.00%> (ø)`
Windows_1	`27.58% <0.00%> (ø)`
Windows_2	`56.53% <100.00%> (ø)`
Windows_3	`38.27% <0.00%> (ø)`
Windows_4	`28.99% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

virajsanghvi

Can you post a screenshot with these changes and the new page header?

virajsanghvi

some comments

virajsanghvi · 2025-07-14T18:50:24Z

src/plugins/home/public/application/components/_home.scss

@@ -28,14 +28,16 @@
 * under the License.
 */

+@import "src/core/public/variables";


Was this already imported in another file? It will likely be imported twice - may want to validate (probably not problematic for this particular file, but not ideal pattern to set)

Was this already imported in another file? It will likely be imported twice - may want to validate (probably not problematic for this particular file, but not ideal pattern to set)

It wasn’t imported elsewhere, but you're right — it probably makes more sense to import it at a higher level like src/plugins/home/public/application/index.scss. I’ll update that, thanks!

virajsanghvi · 2025-07-14T18:52:23Z

src/core/public/chrome/ui/header/header.tsx

+  // Get the banner plugin configuration
+  const bannerPluginConfig = injectedMetadata
+    ?.getPlugins()
+    ?.find((plugin: { id: string }) => plugin.id === 'banner')?.config;
+  const isBannerEnabled = bannerPluginConfig?.enabled === true;


Isn't it a little weird for core to know about a particular plugin? Is idea that if someone else wanted to implement a banner plugin they'd have to give it the same id?

Looks like this is pre-existing code, so not a blocker

Isn't it a little weird for core to know about a particular plugin? Is idea that if someone else wanted to implement a banner plugin they'd have to give it the same id?

Looks like this is pre-existing code, so not a blocker

You're right — it is a bit awkward for core to reference a specific plugin. In this case, it’s needed to ensure the plugin mount is only added to the DOM when enabled, and to conditionally apply the wrapper class.

Definitely open to better approaches if you have something in mind — would love to hear your thoughts!

Do you have a point of view of why this doesn't use the banner overlay service? e.g. curious what answer to https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9989/files#r2193589565 is (@kavilla)

Read the RFC (#9861) - given our reasoning this is standalone and not intended to be reused, not sure I fully understand why this is a plugin if its has a specific purpose that seems coupled to core.

Anyways, @zhongnansu is this a common practice for plugins within core to be referenced by id in core code?

Anyways, @zhongnansu is this a common practice for plugins within core to be referenced by id in core code?

@virajsanghvi There are some options to reference plugin from core, the current implementation is a lightweight solution that's acceptable to me base on the use case. Tho it might not be the best practice.

One option is like implementation in this PR. Although I didn't find similar usage, but injectedMetaService -> getPlugin() API does relay the plugin manifest from server side to client side. A use case like this is acceptable to me, which it just reads the feature flag and conditionally render UI.

A more common way is defining a interface in core and expose to plugin to register/set.

Which approach do you think we should take in this case?

I think the latter - I don't think core should really reference plugins by id (and from your comment, it seems like we don't currently do this), unless the design intent is that users can provide a plugin with this id for this behavior (although the banner plugin is a core plugin, so that seems like its not the case).

It is also odd that we're introducing a core plugin that doesn't support the direction the ux is going (new header) - is this a stop-gap solution? Do we plan to eventually support the new header?

virajsanghvi · 2025-07-14T18:53:28Z

src/plugins/banner/public/index.scss

@@ -19,47 +25,22 @@
  top: 0;
  left: 0;
  right: 0;
-  z-index: 1100; /* Higher than header z-index (1000) */
+  z-index: 1100;


why remove comment?

why remove comment?

This comment was brought in from my previous PR. After reviewing similar cases in the codebase, I realized that z-index values typically aren’t commented this way — so I removed it for consistency.

Ah, didn't realize this was all new code

virajsanghvi · 2025-07-14T19:02:59Z

src/plugins/banner/public/index.scss

-.euiOverlayMask {
-  top: calc(var(--global-banner-height) + $euiSizeXL + $euiSizeM + $euiSizeXL + $euiSizeM) !important;
+.headerGlobalNav--withBanner .primaryHeader:not(.newTopNavHeader) {
+  top: calc(var(--global-banner-height) + $headerWidth);
 }
-/* stylelint-enable @osd/stylelint/no_modifying_global_selectors */


Why is this no longer necessary?

@virajsanghvi
Why is this no longer necessary?

It’s no longer necessary because body content, flyouts, and overlay masks now use shared SCSS variables like $osdHeaderOffset and $osdHeaderOffsetExpanded. These are injected into min-height, padding, and top styles, so layout shifts automatically when the banner appears.

These changes are all related to concerns raised by the already merged PR #9989.

virajsanghvi · 2025-07-14T20:14:32Z

src/plugins/banner/public/index.scss

-.headerIsExpanded.euiBody--headerIsFixed {
-  padding-top: calc(var(--global-banner-height) + $euiSizeXL + $euiSizeM + $euiSizeXL + $euiSizeM) !important;


why is this no longer necessary?

It’s no longer necessary because body content, flyouts, and overlay masks now use shared SCSS variables like $osdHeaderOffset and $osdHeaderOffsetExpanded. These are injected into min-height, padding, and top styles, so layout shifts automatically when the banner appears.

These changes are all related to concerns raised by the already merged PR #9989.

Same as above.

sidwang42 · 2025-07-14T20:59:55Z

@virajsa
Can you post a screenshot with these changes and the new page header?

The default home screen:

The new page Header:

The current banner overlaps with the new header — I plan to address this in a follow-up PR. Supporting a non-overlapping banner with the new header would require adding conditional logic in the SCSS file, which feels out of scope for this PR’s purpose.

zhongnansu

thanks for removing the css override, re-enabling linter

Signed-off-by: Sid Wang <[email protected]>

virajsanghvi · 2025-07-15T18:43:19Z

I plan to address this in a follow-up PR

Why are we following up vs just fixing this now?

sidwang42 · 2025-07-15T23:13:52Z

I plan to address this in a follow-up PR

Why are we following up vs just fixing this now?

Thanks for the question!

I spent a good amount of time investigating possible solutions here. Unlike the default home page, the new home page and its flyout are implemented as fixed-position components. There’s currently no equivalent to $osdHeaderOffset that can be set to adjust their position when the banner is visible.

The primary goal of this project is to promote new OpenSearch Dashboards features (like the new home page) to existing users. Supporting banner offsets in the new home page is out of scope for this PR.

The only viable approach I found involves overriding top-level layout styles like:

.content,
.euiFlyout,
.newPageTopNavExpander {
  top: var(--global-banner-height);
}

However, this would override existing styles and risk regressions.

To avoid that, this PR conditionally hides the banner when the new home page is enabled. A proper solution is tracked in the issue, which will include a refactor of the new UI layout to introduce a --new-ui-header-offset (or equivalent) and apply it systematically across all affected components.

virajsanghvi · 2025-07-17T18:18:18Z

The primary goal of this project is to promote new OpenSearch Dashboards features (like the new home page) to existing users. Supporting banner offsets in the new home page is out of scope for this PR.

If the direction of the new experience is with the new header, it is odd to me that we don't support it.

virajsanghvi · 2025-07-17T18:19:53Z

src/core/public/chrome/ui/header/header.tsx

  return (
    <>
-      {isBannerEnabled && <div id="pluginGlobalBanner" />}
+      {isBannerEnabled && !useUpdatedHeader && <div id="pluginGlobalBanner" />}


Why not always just have a space to mount global banners vs making it specific to this plugin?

virajsanghvi · 2025-07-17T18:22:06Z

src/core/public/chrome/ui/header/header.tsx

+  // Get the banner plugin configuration
+  const bannerPluginConfig = injectedMetadata
+    ?.getPlugins()
+    ?.find((plugin: { id: string }) => plugin.id === 'banner')?.config;
+  const isBannerEnabled = bannerPluginConfig?.enabled === true;


I think the latter - I don't think core should really reference plugins by id (and from your comment, it seems like we don't currently do this), unless the design intent is that users can provide a plugin with this id for this behavior (although the banner plugin is a core plugin, so that seems like its not the case).

It is also odd that we're introducing a core plugin that doesn't support the direction the ux is going (new header) - is this a stop-gap solution? Do we plan to eventually support the new header?

virajsanghvi

Discussed offline, approving given:

This has already been committed, and this improves the current implementation
We plan on picking up new header support and this pr prevents this from showing there
We can address proper extensibility model after this pr (as it fixes something already in the code base) - let's please just have a quick discussion with stakeholders.

One call out, can you validate that failing CI isn't an issue?

zhongnansu

Let's make sure you are tracking all the follow-up properly in separate github issues and and linked here.

sidwang42 · 2025-07-17T21:09:07Z

One call out, can you validate that failing CI isn't an issue?

Build and test / Build min release artifacts on Windows x64 is failing because of:
worker stderr [BABEL] Note: The code generator has deoptimised the styling of D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\artifacts\src\plugins\data\public\antlr\opensearch_sql.generated\OpenSearchSQLParser.ts as it exceeds the max of 500KB

Build and test / Run functional tests on Windows (ciGroup4) (pull_request) is failing after 7m is failing because of:
build_opensearch_dashboards_platform_plugins.js

None of these two could be caused by this PR.
Thanks, for pointing out.

sidwang42 · 2025-07-17T21:25:27Z

We plan on picking up new header support and this pr prevents this from showing there

This will be tracked by this issue:
#10139

We can address proper extensibility model after this pr (as it fixes something already in the code base) - let's please just have a quick discussion with stakeholders.

This will be tracked by this issue:
#10223

github-actions bot added the repeat-contributor label Jul 11, 2025

opensearch-changeset-bot bot added a commit to sidwang42/OpenSearch-Dashboards that referenced this pull request Jul 11, 2025

Changeset file for PR opensearch-project#10125 created/updated

5f5ab13

zhongnansu added the banner Dashboards banner plugin which displays global banner and allows customization label Jul 11, 2025

virajsanghvi reviewed Jul 14, 2025

View reviewed changes

sidwang42 mentioned this pull request Jul 14, 2025

[Meta][Banner Plugin] Feature, bug fix and enhancement for Banner Plugin #9990

Open

6 tasks

virajsanghvi reviewed Jul 14, 2025

View reviewed changes

sidwang42 mentioned this pull request Jul 14, 2025

[Banner Plugin] Working static banner with no overlapping #9989

Merged

2 tasks

zhongnansu requested a review from virajsanghvi July 15, 2025 17:09

zhongnansu previously approved these changes Jul 15, 2025

View reviewed changes

[Split] Extract SCSS and header.tsx changes cleanly

220a618

Signed-off-by: Sid Wang <[email protected]>

sidwang42 dismissed zhongnansu’s stale review via 220a618 July 15, 2025 17:19

sidwang42 force-pushed the feature/scss-header-only-clean branch from 5f5ab13 to 220a618 Compare July 15, 2025 17:19

zhongnansu self-requested a review July 15, 2025 17:23

Merge branch 'main' into feature/scss-header-only-clean

1e81240

sidwang42 force-pushed the feature/scss-header-only-clean branch from 374eae7 to 1e81240 Compare July 15, 2025 22:55

virajsanghvi reviewed Jul 17, 2025

View reviewed changes

Merge branch 'main' into feature/scss-header-only-clean

06e7911

virajsanghvi approved these changes Jul 17, 2025

View reviewed changes

zhongnansu approved these changes Jul 17, 2025

View reviewed changes

zhongnansu merged commit ae1adcb into opensearch-project:main Jul 17, 2025
70 of 73 checks passed

		.headerIsExpanded.euiBody--headerIsFixed {
		padding-top: calc(var(--global-banner-height) + $euiSizeXL + $euiSizeM + $euiSizeXL + $euiSizeM) !important;

[Banner Plugin] UI Implementation Refactor #10125

[Banner Plugin] UI Implementation Refactor #10125

Conversation

sidwang42 commented Jul 11, 2025 • edited by zhongnansu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Screenshot

Implementation Details

Banner Mounting

Header Wrapping

Shared Offsets

Fallback Safety

Testing the changes

Changelog

Check List

Uh oh!

codecov bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

virajsanghvi left a comment

Choose a reason for hiding this comment

Uh oh!

virajsanghvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virajsanghvi Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongnansu Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidwang42 Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidwang42 Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidwang42 commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongnansu left a comment

Choose a reason for hiding this comment

Uh oh!

virajsanghvi commented Jul 15, 2025

Uh oh!

sidwang42 commented Jul 15, 2025

Uh oh!

virajsanghvi commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidwang42 commented Jul 11, 2025 •

edited by zhongnansu

Loading

codecov bot commented Jul 11, 2025 •

edited

Loading

virajsanghvi Jul 15, 2025 •

edited

Loading

zhongnansu Jul 15, 2025 •

edited

Loading

sidwang42 Jul 14, 2025 •

edited

Loading

sidwang42 Jul 14, 2025 •

edited

Loading

sidwang42 commented Jul 14, 2025 •

edited

Loading

virajsanghvi commented Jul 17, 2025 •

edited

Loading

zhongnansu left a comment •

edited

Loading