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

Dark EuiHeader #3524

Merged
merged 7 commits into from
Jun 1, 2020
Merged

Dark EuiHeader #3524

merged 7 commits into from
Jun 1, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented May 28, 2020

Part 1/3 for next iteration of K8 support

This PR tackles 3 main things:

1. Adds theme prop to EuiHeader

This adds support for providing theme="dark" and the header will display as if it's dark mode no matter the theme. For now it only supports a few child components.

image
image

2. Adds Amsterdam updates to EuiHeader

Mainly fixes some usages of $euiHeaderSize vs $euiHeaderChildSize variables, and removes the borders. This does not yet tackle the breadcrumb updates.

image
image
image

3. Fixes a few minor things in some header sub-components

Like alignments and responsive displays.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Reviewed the code and tested it. It looks good! 🎉

I just added a few suggestions.

iconSide="right">
Production logs
</EuiBadge>,
<EuiHeaderSectionItemButton aria-label="Alerts" notification={'•'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion to show an actual number because I thought the icon was not loading.

Suggested change
<EuiHeaderSectionItemButton aria-label="Alerts" notification={'•'}>
<EuiHeaderSectionItemButton aria-label="Alerts" notification={1}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how alerts work in Kibana, originally a design from @ryankeairns. It is probably because the mechanism wouldn't necessarily know how many items were unread in the alerts (notifications) panel.

* The `default` will inherit its coloring from the light or dark theme.
* Or, force the header into pseudo `dark` theme for all themes.
*/
theme?: 'default' | 'dark';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking if we should have here 'default' | 'dark' | 'light';

The default would inherit its coloring from the active theme. But If I have for instance the dark theme active and I want to use the light nav I could pass theme="light". But I'm not sure if it's a valid use case. So it's just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had thought about this too, but it's yet another series of overrides that is complicated right now with the way our SASS works. I'm hoping that when we have CSS in JS, we will be able to do this with a quick flip of a prop and would make doing this a lot easier. For now, I think I'm just trying to support our needs for Kibana and we can add a "Light always" version later.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the css-in-js future, wouldn't the values more correctly by 'default' | 'inverse'? If dark theme is enabled, the header should swap to light?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, the way the designers want this setup is to force dark mode always. It will not invert to white in dark mode.

@miukimiu
Copy link
Contributor

Noticed now that a changelog is missing.

@ryankeairns
Copy link
Contributor

Looking great, one very minor docs consideration - we're going to change the What's New icon from email to cheer if you'd like to align the example to Kibana :)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Code changes LGTM; Tested in the PR's docs deploy

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Checked over code and functionality.

@cchaos cchaos merged commit 2e7f6f2 into elastic:master Jun 1, 2020
@cchaos cchaos deleted the k8/dark__header branch June 1, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants