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

Update OuiCollapsableNav > OuiCollapsableNavGroup background to use the same value as OuiPageBackgroundColor #904

Closed
Tracked by #895
KrooshalUX opened this issue Jul 20, 2023 · 8 comments · Fixed by #968
Assignees

Comments

@KrooshalUX
Copy link
Contributor

No description provided.

@joshuarrrr
Copy link
Member

@KrooshalUX The OuiCollapsibleNavGroup actually takes a background prop which sets theme-defined backgrounds and can be none (default), dark, or light:

Screen.Recording.2023-07-20.at.4.59.51.PM.mov

I'm wondering if we can deprecate this if that's not something you want exposed to designers and builders. If we keep it as is, we'd need to define the behavior of each prop setting per theme.

@KrooshalUX
Copy link
Contributor Author

KrooshalUX commented Jul 21, 2023

I don't see this same interface where you see "set background" - but I have seen the "none" "dark" "light" array listed in props. So, this issue is about changing "dark" and "light" to the same value as OuiPageBackgroundColor, if that is a more explicit ask. I am not sure what happens when the background is set to "none" and the collapsible nav is undocked. - but if "none" just ends up using OuiPageBackgroundColor, then we can satisfy this change with deprecating the prop.

@joshuarrrr
Copy link
Member

Here are the background definitions:

$ouiCollapsibleNavGroupLightBackgroundColor: $ouiPageBackgroundColor !default;
$ouiCollapsibleNavGroupDarkBackgroundColor: lightOrDarkTheme(
shade($ouiColorDarkestShade, 20%),
shade($ouiColorLightestShade, 50%),
) !default;
$ouiCollapsibleNavGroupDarkHighContrastColor: makeGraphicContrastColor(
$ouiColorPrimary,
$ouiCollapsibleNavGroupDarkBackgroundColor
) !default;

Here are the color values in next_dark:

  "ouiCollapsibleNavGroupLightBackgroundColor": "#172430",
  "ouiCollapsibleNavGroupDarkBackgroundColor": "#0d1720",
  "ouiCollapsibleNavGroupDarkHighContrastColor": "#159d8d",
  "euiCollapsibleNavGroupLightBackgroundColor": "#172430",
  "euiCollapsibleNavGroupDarkBackgroundColor": "#0d1720",
  "euiCollapsibleNavGroupDarkHighContrastColor": "#159d8d",

Here are the color values in next_light:

  "ouiCollapsibleNavGroupLightBackgroundColor": "#f0f2f4",
  "ouiCollapsibleNavGroupDarkBackgroundColor": "#24313e",
  "ouiCollapsibleNavGroupDarkHighContrastColor": "#159d8d",
  "euiCollapsibleNavGroupLightBackgroundColor": "#f0f2f4",
  "euiCollapsibleNavGroupDarkBackgroundColor": "#24313e",
  "euiCollapsibleNavGroupDarkHighContrastColor": "#159d8d",

@joshuarrrr
Copy link
Member

joshuarrrr commented Aug 2, 2023

The OpenSearch Dashboards sidebar menu is composed of multiple collapsible nav group components, a couple of which are not commonly (or ever) used:

Screenshot 2023-08-02 at 16-24-19 Dashboards - OpenSearch Dashboards copy

  1. Custom Nav Link (no known usage)
  2. Recently viewed
  3. Categorized sections
  4. Uncategorized nav links (if a plugin does not correctly provide a category when adding a nav link - should not be used)
  5. Docking controls

Note that groups 1. and 2. are currently visually distinguished from the rest with a different background shade. Additionally, they're not part of the same scroll container as everything else.
Screenshot 2023-08-02 at 16-24-03 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 16-24-19 Dashboards - OpenSearch DashboardsScreenshot 2023-08-02 at 16-25-15 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 16-24-54 Dashboards - OpenSearch Dashboards

While I think it's relatively safe to ignore 1. and 4., because we have no evidence of their use in the wild, I included them in the example screenshots for considering the overall component design

Taking into account all of the above, my recommended solution is to

  1. Update all <EuiCollapsibleNavGroup> components in OpenSearch Dashboard to use the set the background prop to light (which is already defined to use $ouiPageBackgroundColor for all themes).
  2. Mark as deprecated the dark option for the background prop (because we have no examples of any usage of it)

This change would result in all sections using the OuiPageBackgroundColor (including in existing themes):

Screenshot 2023-08-02 at 15-12-01 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-12-29 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-13-24 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-13-05 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-14-03 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-14-31 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-15-24 Dashboards - OpenSearch Dashboards
Screenshot 2023-08-02 at 15-15-09 Dashboards - OpenSearch Dashboards

The advantage is that we don't need to introduce theme-specific logic to this component that currently has none, and we retain flexibility to use the alternative background options in other context (the only other usage of <EuiCollapsibleNavGroup> today is in the maps plugin): https://github.com/search?q=org%3Aopensearch-project+EuiCollapsibleNavGroup+NOT+repo%3Aopensearch-project%2Foui+NOT+repo%3Aopensearch-project%2FOpenSearch-Dashboards&type=code

The potential disadvantage is that it changes (subtly) the look of the nav menu existing themes, although we may consider that to be an improvement.

@KrooshalUX
Copy link
Contributor Author

I don't know that I want the same visual effect to be applied to the current theme. In V7, the background color giving contrast to OuiCollapsibleNav is appropriate, whereas in Next, its less in-line with the overall look & feel change. The collapsible nav & header are meant to appear less "chrome-heavy" in the Next theme, thus matching the page background is ideal to balance the focus to the content. I don't know that the same approach works on v7.

Can you update the screenshots to include a next dark version with the nav docked? Its hard to tell the outcome of the background apporach with it floating and on the scrim.

@joshuarrrr joshuarrrr moved this to In Progress in Look & Feel Aug 9, 2023
@joshuarrrr joshuarrrr moved this from In Progress to UX Required in Look & Feel Aug 10, 2023
@joshuarrrr joshuarrrr moved this from UX Required to In Progress in Look & Feel Aug 10, 2023
@joshuarrrr
Copy link
Member

Screenshots updated.

@joshuarrrr
Copy link
Member

joshuarrrr commented Aug 16, 2023

@KrooshalUX Some more context and findings I wanted to share. In general, the background color you're seeing for most of the sidenav (OuiCollapsibleNav > OuiCollapsibleNavGroup background) is not set by either of those components, because the OuiCollapsibleNavGroup component only sets a background if the background prop is set to dark or light. Otherwise, the background is just set by parent components.

Because OuiCollapsibleNav is just a special-purpose wrapper of OuiFlyout, the background styling is coming from OuiFlyout, which sets a background color of $ouiColorEmptyShade:

background: $ouiColorEmptyShade;

Not sure that changes the particular guidance here at all, but it's something to consider if you want all flyouts to have a more neutral PageBackground color, or for that to be separately theme-able.

For now, I'm thinking of refactoring so that we can treat OuiCollapsibleNav as a special case with its own background behavior.

@joshuarrrr joshuarrrr moved this from In Progress to UX Required in Look & Feel Aug 16, 2023
@joshuarrrr joshuarrrr moved this from UX Required to In Review in Look & Feel Aug 17, 2023
@github-project-automation github-project-automation bot moved this from In Review to Done in Look & Feel Aug 17, 2023
@joshuarrrr
Copy link
Member

See also #1016 - I ended up adding an additional themable override to the background of OuiCollapsibleNav for themes (like next) that would like to style it separately from OuiFlyout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment