Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3524/ |
elizabetdev
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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/ |
chandlerprall
left a comment
There was a problem hiding this comment.
Code changes LGTM; Tested in the PR's docs deploy
snide
left a comment
There was a problem hiding this comment.
Checked over code and functionality.
Part 1/3 for next iteration of K8 support
This PR tackles 3 main things:
1. Adds
themeprop 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
$euiHeaderSizevs$euiHeaderChildSizevariables, 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