Skip to content

Comments

[Side Nav] Styling fixes for panel openers#218513

Merged
tsullivan merged 25 commits intoelastic:mainfrom
tsullivan:sidenav/fix-icons-for-panelopeners
Apr 30, 2025
Merged

[Side Nav] Styling fixes for panel openers#218513
tsullivan merged 25 commits intoelastic:mainfrom
tsullivan:sidenav/fix-icons-for-panelopeners

Conversation

@tsullivan
Copy link
Member

@tsullivan tsullivan commented Apr 17, 2025

Summary

Epic: https://github.com/elastic/kibana-team/issues/1439

For "panel opener" type of nav items:

  1. Allow buttons for "panel opener" nav items to show the icon.
  2. Adjust the size of the arrow for panel opener
  3. Flips the direction the arrow points when the secondary nav is expanded

For all types of nav items:

  1. Fix highlighted/hover colors

Other changes:

  1. Refactored Emotion code to separate "static" and "dynamic" CSS rules.

Not addressed:

  1. Long nav item title text that needs truncation and alignment fixes
  2. Title text should never be underlined

Screenshots

Icon fixes

Before: "Item 03", "Item 05", and "Item 06" were not showing the icon to the left of the title. The "Item 06" is showing an incorrect background color for the highlighted state.
image

After: The icons are fixed, and the highlighted state color is fixed
image

Panel fixes

The background color of the "selected" nav item in the panel is fixed. More panel styling fixes are coming soon!
image

@tsullivan tsullivan force-pushed the sidenav/fix-icons-for-panelopeners branch from 877c835 to 4b6c1cd Compare April 17, 2025 21:47
@tsullivan tsullivan force-pushed the sidenav/fix-icons-for-panelopeners branch from 4b6c1cd to 55ec06f Compare April 17, 2025 21:49
@tsullivan tsullivan force-pushed the sidenav/fix-icons-for-panelopeners branch 2 times, most recently from 83c4153 to 60e01c8 Compare April 17, 2025 21:52
@tsullivan tsullivan force-pushed the sidenav/fix-icons-for-panelopeners branch from 60e01c8 to 4feb1a3 Compare April 17, 2025 21:54
@tsullivan tsullivan changed the title [Side Nav] Support for panel openers to show icon [Side Nav] Styling fixes for panel openers Apr 17, 2025
@tsullivan tsullivan marked this pull request as ready for review April 17, 2025 22:00
@tsullivan tsullivan requested a review from a team as a code owner April 17, 2025 22:00
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting design labels Apr 17, 2025
Copy link
Contributor

@ek-so ek-so left a comment

Choose a reason for hiding this comment

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

I'd not flip the arrow — it looks fine when the arrow is placed horizontally, like an accordion, but in this case tbh looks a little bit confusing. (I believe, it was not mentioned in requirements?)

@tsullivan tsullivan force-pushed the sidenav/fix-icons-for-panelopeners branch from 79d6bfe to 65f248d Compare April 22, 2025 23:17
@tsullivan tsullivan requested a review from a team April 22, 2025 23:44
@Dosant
Copy link
Contributor

Dosant commented Apr 23, 2025

Highlighting works well, but I think I noticed a small regression with a gap on some items.

Screenshot 2025-04-23 at 14 15 03

@tsullivan
Copy link
Member Author

@Dosant I've fixed the gap problem with the "panel opener" buttons
image

@ek-so I've fixed the issue you found in my next PR, here: #218050 (review). It is visible in the storybook of this PR.
image

Copy link
Contributor

@eokoneyo eokoneyo left a comment

Choose a reason for hiding this comment

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

Tested this out in storybook, there's some things I noticed;

I might be mistaken but I think this standalone menu item should line up with the text and icon start, also it protrudes out of it's own hover highlight.
Screenshot 2025-04-26 at 00 18 09

I also noticed that the nested menu item here, exceeds the left margin of it's parent and isn't aligned with it's siblings
Screenshot 2025-04-26 at 00 16 56

@ryankeairns
Copy link
Contributor

ryankeairns commented Apr 30, 2025

@tsullivan should any of these custom styles be applied at the EUI level? (e.g. EuiCollapsibleNavItem)

Not asking to block the PR, but I am thinking some of these should ultimately live there and, once added, we could remove the custom styles that get merged in here. If so, the process would be to open an issue in EUI and reference the custom styles from this PR.

Copy link
Contributor

@ek-so ek-so left a comment

Choose a reason for hiding this comment

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

As we discussed — a couple of things will get together in another PR to make a whole picture

@tsullivan
Copy link
Member Author

tsullivan commented Apr 30, 2025

@tsullivan should any of these custom styles be applied at the EUI level? (e.g. EuiCollapsibleNavItem)

Not asking to block the PR, but I am thinking some of these should ultimately live there and, once added, we could remove the custom styles that get merged in here. If so, the process would be to open an issue in EUI and reference the custom styles from this PR.

Hi @ryankeairns, this is an excellent consideration. Many of the updates can't be applied at the EUI level: the "panel openers" are a customization that lives in Kibana and is built "on top" of the EUI component. But there are also changes to accordions and regular nav items that are part of the EUI component. I will file an issue to place the appropriate styles in the EUI repo and remove the customizations from this Kibana package. Thank you!

Update: filed #219751

@tsullivan
Copy link
Member Author

As we discussed — a couple of things will get together in another PR to make a whole picture

This refers to: #218050. Parts of the footer have invalid styles because footer items currently have an invalid structure.

@tsullivan tsullivan enabled auto-merge (squash) April 30, 2025 14:56
@tsullivan tsullivan merged commit d36a1da into elastic:main Apr 30, 2025
11 checks passed
@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
navigation 44 45 +1
serverless 40 41 +1
total +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
navigation 18.7KB 20.3KB +1.6KB
serverless 19.4KB 21.0KB +1.6KB
total +3.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
navigation 11.9KB 11.9KB -2.0B

History

pborgonovi pushed a commit that referenced this pull request Apr 30, 2025
## Summary

Epic: elastic/kibana-team#1439

For "panel opener" type of nav items:
1. Allow buttons for "panel opener" nav items to show the icon.
2. Adjust the size of the arrow for panel opener
3. Flips the direction the arrow points when the secondary nav is
expanded

For all types of nav items:
1. Fix highlighted/hover colors

Other changes:
1. Refactored Emotion code to separate "static" and "dynamic" CSS rules.

**Not** addressed:
1. Long nav item title text that needs truncation and alignment fixes
2. Title text should never be underlined

### Screenshots

#### Icon fixes
**Before:** "Item 03", "Item 05", and "Item 06" were not showing the
icon to the left of the title. The "Item 06" is showing an incorrect
background color for the highlighted state.
<img width="826" alt="image"
src="https://github.com/user-attachments/assets/8b5b57b7-398c-4a7e-9c93-ad549efd76f7"
/>

**After:** The icons are fixed, and the highlighted state color is fixed
<img width="833" alt="image"
src="https://github.com/user-attachments/assets/94200dd9-b860-4e5d-9bec-ff9d0698a9e1"
/>

#### Panel fixes
The background color of the "selected" nav item in the panel is fixed.
**More panel styling fixes are coming soon!**
<img width="826" alt="image"
src="https://github.com/user-attachments/assets/a9588f3c-8a65-40c0-90fb-5d55de0ceb59"
/>
tsullivan added a commit that referenced this pull request May 9, 2025
## Summary

Epic: elastic/kibana-team#1439
Depends on #218513

### List of changes
1. Match header text style with the main panel
2. Match spacing between items with the main panel
3. Show horizontal line between groups in the panel
- This restores a custom error that was in the code prior to #218156,
but adds better handling and tests to the error
5. Fix position of accordion group arrows in the secondary panel
(viewable only from Storybook)
6. isOpen is not really needed and can be cleaned up
- (see
https://github.com/elastic/kibana/pull/217708/files#diff-9e95eb3e4be14fb61caca86b48be4f03ca63393eeeadcd0b36f7371958a7e5e1L55)
7. (not panel-related) remove the on-hover and on-focus underline from
nav items

### Checklist from Kate:
- [x] text font-weight of solution title in main panel
- [x] text font-weight of secondary panel link content
- [x] zero space between items
- [x] use ~~12px~~ nice-looking margin from the link to the edge of the
panel
- [x] ensure the divider reaches to the edge of the panel
- [x] increase space from panel header to the items
- [x] ensure the width of both panels is 248px
- [x] secondary panel header should vertically align with the below nav
items
- [x] (not panel-related) font color and icon color of selected items
should be "primary" blue

### Screenshots

**Security Solution Serverless**


https://github.com/user-attachments/assets/83ca1c25-f44a-4270-96a8-e28e7b6d7041

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025
## Summary

Epic: elastic/kibana-team#1439

For "panel opener" type of nav items:
1. Allow buttons for "panel opener" nav items to show the icon.
2. Adjust the size of the arrow for panel opener
3. Flips the direction the arrow points when the secondary nav is
expanded

For all types of nav items:
1. Fix highlighted/hover colors

Other changes:
1. Refactored Emotion code to separate "static" and "dynamic" CSS rules.

**Not** addressed:
1. Long nav item title text that needs truncation and alignment fixes
2. Title text should never be underlined

### Screenshots

#### Icon fixes
**Before:** "Item 03", "Item 05", and "Item 06" were not showing the
icon to the left of the title. The "Item 06" is showing an incorrect
background color for the highlighted state.
<img width="826" alt="image"
src="https://github.com/user-attachments/assets/8b5b57b7-398c-4a7e-9c93-ad549efd76f7"
/>

**After:** The icons are fixed, and the highlighted state color is fixed
<img width="833" alt="image"
src="https://github.com/user-attachments/assets/94200dd9-b860-4e5d-9bec-ff9d0698a9e1"
/>

#### Panel fixes
The background color of the "selected" nav item in the panel is fixed.
**More panel styling fixes are coming soon!**
<img width="826" alt="image"
src="https://github.com/user-attachments/assets/a9588f3c-8a65-40c0-90fb-5d55de0ceb59"
/>
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025
## Summary

Epic: elastic/kibana-team#1439
Depends on elastic#218513

### List of changes
1. Match header text style with the main panel
2. Match spacing between items with the main panel
3. Show horizontal line between groups in the panel
- This restores a custom error that was in the code prior to elastic#218156,
but adds better handling and tests to the error
5. Fix position of accordion group arrows in the secondary panel
(viewable only from Storybook)
6. isOpen is not really needed and can be cleaned up
- (see
https://github.com/elastic/kibana/pull/217708/files#diff-9e95eb3e4be14fb61caca86b48be4f03ca63393eeeadcd0b36f7371958a7e5e1L55)
7. (not panel-related) remove the on-hover and on-focus underline from
nav items

### Checklist from Kate:
- [x] text font-weight of solution title in main panel
- [x] text font-weight of secondary panel link content
- [x] zero space between items
- [x] use ~~12px~~ nice-looking margin from the link to the edge of the
panel
- [x] ensure the divider reaches to the edge of the panel
- [x] increase space from panel header to the items
- [x] ensure the width of both panels is 248px
- [x] secondary panel header should vertically align with the below nav
items
- [x] (not panel-related) font color and icon color of selected items
should be "primary" blue

### Screenshots

**Security Solution Serverless**


https://github.com/user-attachments/assets/83ca1c25-f44a-4270-96a8-e28e7b6d7041

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
qn895 pushed a commit to qn895/kibana that referenced this pull request Jun 3, 2025
## Summary

Epic: elastic/kibana-team#1439
Depends on elastic#218513

### List of changes
1. Match header text style with the main panel
2. Match spacing between items with the main panel
3. Show horizontal line between groups in the panel
- This restores a custom error that was in the code prior to elastic#218156,
but adds better handling and tests to the error
5. Fix position of accordion group arrows in the secondary panel
(viewable only from Storybook)
6. isOpen is not really needed and can be cleaned up
- (see
https://github.com/elastic/kibana/pull/217708/files#diff-9e95eb3e4be14fb61caca86b48be4f03ca63393eeeadcd0b36f7371958a7e5e1L55)
7. (not panel-related) remove the on-hover and on-focus underline from
nav items

### Checklist from Kate:
- [x] text font-weight of solution title in main panel
- [x] text font-weight of secondary panel link content
- [x] zero space between items
- [x] use ~~12px~~ nice-looking margin from the link to the edge of the
panel
- [x] ensure the divider reaches to the edge of the panel
- [x] increase space from panel header to the items
- [x] ensure the width of both panels is 248px
- [x] secondary panel header should vertically align with the below nav
items
- [x] (not panel-related) font color and icon color of selected items
should be "primary" blue

### Screenshots

**Security Solution Serverless**


https://github.com/user-attachments/assets/83ca1c25-f44a-4270-96a8-e28e7b6d7041

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting design release_note:skip Skip the PR/issue when compiling release notes v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants