-
Notifications
You must be signed in to change notification settings - Fork 844
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
Dark EuiHeader #3524
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/ |
There was a problem hiding this 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={'•'}> |
There was a problem hiding this comment.
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.
<EuiHeaderSectionItemButton aria-label="Alerts" notification={'•'}> | |
<EuiHeaderSectionItemButton aria-label="Alerts" notification={1}> |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Noticed now that a changelog is missing. |
Looking great, one very minor docs consideration - we're going to change the What's New icon from |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/ |
There was a problem hiding this 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
There was a problem hiding this 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.
Part 1/3 for next iteration of K8 support
This PR tackles 3 main things:
1. Adds
theme
prop to EuiHeaderThis 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.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.3. Fixes a few minor things in some header sub-components
Like alignments and responsive displays.
Checklist
[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes