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

Theme (Header) - update next background #936

Conversation

joshuarrrr
Copy link
Member

@joshuarrrr joshuarrrr commented Jul 28, 2023

Description

Updates default header background in next theme (dark and light) to use the page background.

Issues Resolved

Fixes #901

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

OUI doc site examples

Note that there is no change to existing theme behavior, screenshots provided just for reference and verification

Screenshot 2023-08-15 at 12-34-57 OpenSearch UI
Screenshot 2023-08-15 at 12-35-15 OpenSearch UI
Screenshot 2023-08-15 at 12-35-54 OpenSearch UI
Screenshot 2023-08-15 at 12-36-34 OpenSearch UI

Examples that demonstrate how this background will appear with the defined dark header above (the default layout):

Note that there is no change to existing theme behavior, screenshots provided just for reference and verification

Screenshot 2023-08-15 at 13-30-19 Visual Consistency Dashboard - OpenSearch Dashboards
Screenshot 2023-08-15 at 13-31-09 Visual Consistency Dashboard - OpenSearch Dashboards
Screenshot 2023-08-15 at 13-38-00 Visual Consistency Dashboard - OpenSearch Dashboards
Screenshot 2023-08-15 at 13-39-48 Visual Consistency Dashboard - OpenSearch Dashboards

Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
@joshuarrrr
Copy link
Member Author

Awaiting approval from @KrooshalUX

@KrooshalUX
Copy link
Contributor

The effect isn't coming through when the stacked nav is present. I would expect that the background changes for both layers of the stacked nav. Is it possible to show examples of that? Since we don't have telemetry on how many people use stacked vs condensed nav, its a little tough to make the call on the color just impacting one line of the header.

@joshuarrrr
Copy link
Member Author

joshuarrrr commented Aug 7, 2023

The effect isn't coming through when the stacked nav is present. I would expect that the background changes for both layers of the stacked nav.

No, that's not expected, because we didn't change the colors for the "dark" (prop, not theme) variant of the nav - see #901 (comment) and the comment below it for the actual values

Is it possible to show examples of that?

I'm not following what you'd like to see an example of - can you provide either color values or mocks?

Since we don't have telemetry on how many people use stacked vs condensed nav, its a little tough to make the call on the color just impacting one line of the header.

Because the stacked nav is still the default, the vast majority of users will see it, unless they've cared enough to change the settings since it was introduced in 2.1.0. See opensearch-project/OpenSearch-Dashboards#1834 for options on making the condensed view more common for users.

@KrooshalUX
Copy link
Contributor

KrooshalUX commented Aug 8, 2023

@joshuarrrr Here is a mock up of both sidebar and header with the background as the same as the page color

image

@joshuarrrr
Copy link
Member Author

@KrooshalUX I've updated the change and also the screenshots, so ready for your re-review.

One note - after discussing with @AMoo-Miki and @BSFishy, we think it's important (for now) to make sure that the next light theme still respects a dark header theme. Instead, the dark header background will be the same as the next dark page background color. The reason we can't really use the next light page background color is that the dark header prop also sets lots of styles on the children. It would make most sense to deprecate that once we drop the current theme, drop support for the expanded header, or both.

@KrooshalUX
Copy link
Contributor

Given the latest discussions w/engineering, LGTM for now. I think we can continue discussions on the impact of the dark header in subsequent releases.

@KrooshalUX
Copy link
Contributor

KrooshalUX commented Aug 16, 2023

CC @kgcreative for awareness on outcome.

@joshuarrrr
Copy link
Member Author

@kgcreative Any additional UX concerns here based on the screenshots provided?

@joshuarrrr joshuarrrr merged commit fa48112 into opensearch-project:main Aug 17, 2023
6 checks passed
@joshuarrrr joshuarrrr deleted the feat/update-next-header-background branch August 17, 2023 18:14
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 17, 2023
* Theme (Header) - update next background

Signed-off-by: Josh Romero <rmerqg@amazon.com>

* add changelog

Signed-off-by: Josh Romero <rmerqg@amazon.com>

* Update ouiHeaderDarkBackgroundColor in next theme

Signed-off-by: Josh Romero <rmerqg@amazon.com>

---------

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit fa48112)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
joshuarrrr pushed a commit that referenced this pull request Aug 17, 2023
* Theme (Header) - update next background

Signed-off-by: Josh Romero <rmerqg@amazon.com>

* add changelog

Signed-off-by: Josh Romero <rmerqg@amazon.com>

* Update ouiHeaderDarkBackgroundColor in next theme

Signed-off-by: Josh Romero <rmerqg@amazon.com>

---------

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit fa48112)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update OuiHeader background to use the same value as OuiPageBackgroundColor
5 participants