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

Configurable top header colors #1448

Closed
wants to merge 2 commits into from

Conversation

evamillan
Copy link
Contributor

Description

Adds two new configs branding.colors.headerBackgroundColor and branding.colors.headerLinkColor in the yaml file to
make the top header background and link colors configurable.
Valid values are HEX and RGB color codes.

imagen

Issues Resolved

#1368

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Adds two new configs branding.colors.headerBackgroundColor
and branding.colors.headerLinkColor in the yaml file to
make the top header background and link colors configurable.
Valid values are HEX and RGB color codes.

Signed-off-by: Eva Millán <[email protected]>
@evamillan evamillan requested a review from a team as a code owner April 12, 2022 14:18
@tmarkley tmarkley added the ux / ui Improvements or additions to user experience, flows, components, UI elements label Apr 12, 2022
@kgcreative
Copy link
Member

kgcreative commented Apr 12, 2022

Hey @evamillan - Thanks for putting this together!

Have you considered being able to independently set bottom border, as well as background color? (I noticed in the code base you are setting the bottom border to equal the branding.colors.headerBackgroundColor)

Additionally, are there hooks for both Dark Mode AND Light mode themes? If users are providing two versions of the logo, they may want to provide two different header background colors.

@tmarkley - I think the behavior today is that the header is the same color regardless of dark/light mode. This something we should revisit if we're adding themeability to the header, and if that's the case, do we need to revisit the custom branding data structure?

cc: @opensearch-project/opensearch-ux

@kgcreative kgcreative assigned tmarkley and unassigned kgcreative Apr 12, 2022
@evamillan
Copy link
Contributor Author

@kgcreative I didn't take the dark/light mode themes into account because the header is currently hardcoded as "dark", but I can change that and add another config setting for the bottom border as well.

@kgcreative
Copy link
Member

@evamillan That would make this much more future proof. If those configs are blank we can keep the behavior as-is, but if there is light and dark theme definitions, then we can take those into account, particularly if there is dark + light mode logos defined as well.

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening @evamillan!

Temporary blocking to ensure stability for 2.0.0-rc1

Users can set different colors for the top
header background, border and links in the
default or dark mode.

Signed-off-by: Eva Millán <[email protected]>
@evamillan
Copy link
Contributor Author

I updated the PR to add configs for the default and dark modes and another one to set the border color.

@ashwin-pc
Copy link
Member

@evamillan Sorry for taking so long to get back to your PR. I had a few questions and concerns about the implementation:

  1. If none of the config parameters are set for the header, it breaks the way the header looks today.
    Screen Shot 2022-05-02 at 4 15 19 PM

  2. I am not sure this is the best way to customize the header colors. The header colors come from eui variables $euiHeaderBackgroundColor & $euiHeaderDarkBackgroundColor which are directly set on the eui header component. If we want to customize these colors, we should be overriding these variables as opposed to explicitly setting the value for the header. They are set in _header.scss in EUI.

  3. The way branding is setup today is not ideal for theming which is similar to what this PR aims to do. There is an open issue to reduce code duplication for branding [Cleanup] Reduce duplication in code for Initial Custom Branding #895. If we decide to approach branding the header with the current approach, we should resolve that first so that we dont add technical debt to the repo.

I think this feature might need a little more thought/design before implementation. One such solution could be an scss overrides file that is imported before any of the eui styles. This will simplify a lot of the customization. I doubt that the header colors will be the only colors that someone will want to customize.

@kgcreative The dark and light headers are actually not the same. The dark mode header is slightly darker than the light mode one. And i do think we need to revisit the branding data structure. I also think we should separate theming from branding.

@evamillan
Copy link
Contributor Author

Thanks for your comments, @ashwin-pc. If the idea is to reduce the branding code then this approach is probably not the best one.

Regarding the possible solution of an scss overrides file, I've made a small test and noticed the styles are loaded across several packages, with most of them coming from the osd-ui-shared-deps theme files. These are cached and only built with the bootstrap script. Would it make sense to change that and compile them at runtime like the osd-optimizer does?

@kavilla
Copy link
Member

kavilla commented May 23, 2022

@joshuarrrr, @kgcreative could you comment on this with the re-design?

Also, #1368 I believe @ashwin-pc wanted to comment on the design as well.

@ashwin-pc
Copy link
Member

@evamillan Yeah I think you make a good point. Let's have that conversation in #1368?

@ashwin-pc
Copy link
Member

Closing this PR since this issue #1368 will require more work that what is covered in this PR.

@ashwin-pc ashwin-pc closed this Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ux / ui Improvements or additions to user experience, flows, components, UI elements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants