Make sidenav coloring more consistant for open nodes #3926
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3926/ |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
I think this makes a lot of sense. The one thing that still gets me about this component is that I feel like the coloring is backwards. Our typical links use the primary blue colors, but in the side-nav we're indicating current selection in blue instead of all the other links being blue. But I know that it would then probably be a bit overwhelmingly blue.
Probably something we could discuss at a higher scope and don't need to address in this PR.
Thoughts though on maybe bolding the current selection even more? Right now it just goes from 400-500. I think we could take it all the way to 700.
|
|
||
| }} |
|
Good catch. Feedback addressed. Agree with you on the bold. Also think the blue would be too much and that as long as sidenav is used in a common nav place (sidebars) it works with its darker coloring. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3926/ |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Yeah, I guess I was just wondering if the selected one should be blue at all. But again, not pushing to figure that out in this PR.
I did have some suggestions here though to actually use our text-specific color vars instead of the shade color.
|
@cchaos Feedback addressed |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3926/ |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Sorry just caught one more thing. Should we move the coloring of the label to the outer element instead so that the icons can inherit that color?
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3926/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3926/ |
* Add all exported types * Add some common interfaces * Add extends to interfaces * Remove comment * Fixed issue when extendedInterfaces is empty * Use sass variables and remove font weight * Include HTMLElement in Props tab if not superseded by another element * Fixed missing key issue * Fix display of extends (including mobile) Co-authored-by: Chandler Prall <[email protected]> Co-authored-by: cchaos <[email protected]>
* Add vertical scroll and text highlighting for functions * use code block instead of markdown * Swap usages of code for code block and fix overflow * One codeblock with line breaks when multiple functions * Fixed warnings of missing keys Co-authored-by: cchaos <[email protected]>
* Add new ML icons. Prettier updates on app search icons. * Update alignment in all ML icons * Update app search and workplace search icons * Update changelog * Fix changelog entry * Fix changelog entry
|
Bungled git on this one somehow. Closing in favor of #3945 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3926/ |
5 participants
Summary
Minor change to the sidenav coloring to make it more consistant for navs in their various open / close states. The current setup over-assumes that you'll be using open states as a location reference. This might be something we want to change based on a prop, but in general I think this way is a more consistent default.
Before
After
Why this is important
When working with trees where you force them open to show heigharchy, you'd wonder why the coloring was highlighted differently.
Checklist
Checked in mobileChecked in Chrome, Safari, Edge, and FirefoxProps have proper autodocsChecked Code Sandbox works for the any docs examplesAdded or updated jest testsChecked for breaking changes and labeled appropriately