Skip to content

[EuiCollapsibleNavBeta] Fix z-index level to match production EuiCollapsibleNav#8075

Closed
cee-chen wants to merge 2 commits intoelastic:mainfrom
cee-chen:collapsible-nav-beta/z-index
Closed

[EuiCollapsibleNavBeta] Fix z-index level to match production EuiCollapsibleNav#8075
cee-chen wants to merge 2 commits intoelastic:mainfrom
cee-chen:collapsible-nav-beta/z-index

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Oct 14, 2024

Summary

This ensures that the nav sits above any EuiTourStep or other popover elements (otherwise, popovers by default will sit above flyouts). Thank you to @kapral18 for pointing this out to us.

EuiCollapsibleNav styles:

z-index: ${euiTheme.levels.navigation};

VRT screenshots appear to be minute pixel-level changes, I'm just adding them in here to get them updated

QA

General checklist

  • Browser QA
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked in both light and dark modes
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
    - [ ] Added or updated jest and cypress tests
  • Release checklist - N/A, beta component
  • Designer checklist - N/A

@cee-chen cee-chen changed the title [EuiCollapsibleNavBeta] Fix z-index levels to match production EuiCollapsibleNav [EuiCollapsibleNavBeta] Fix z-index level to match production EuiCollapsibleNav Oct 14, 2024
@cee-chen cee-chen added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Oct 14, 2024
@cee-chen cee-chen marked this pull request as ready for review October 14, 2024 21:44
@cee-chen cee-chen requested a review from a team as a code owner October 14, 2024 21:44
@cee-chen
Copy link
Copy Markdown
Contributor Author

@sebelga Just to check, do you see any issues with increasing the z-index of the serverless nav to be above popovers especially when in push / non-mobile mode?

@elasticmachine
Copy link
Copy Markdown
Collaborator

💚 Build Succeeded

@kapral18
Copy link
Copy Markdown
Contributor

hey @cee-chen after thinking a bit I realized that even though this is correct course of action for eui library in isolation, for kibana this will block adoption of eui version that releases this change, because there are a lot of elements currently that are above navigation and after this change will be below, for example the timeline, which currently sits at 1003 z index etc... We need to first fix z-index usage in kibana and correctly position relationships between stacked z-indices across the ui using https://eui.elastic.co/#/theming/more-tokens%23levels as a reference.

cc: @elastic/security-threat-hunting-explore @elastic/security-threat-hunting-investigations

@cee-chen
Copy link
Copy Markdown
Contributor Author

Gotcha, no worries, I can scrap this PR if it will interfere with Kibana. Thank you for investigating this so thoroughly @kapral18!

@sebelga
Copy link
Copy Markdown
Contributor

sebelga commented Oct 16, 2024

Just to check, do you see any issues with increasing the z-index of the serverless nav to be above popovers especially when in push / non-mobile mode?

I wonder if that would impact this popover (tour) that we added elastic/kibana#194926. Althoug it is declared inside the header.

@cee-chen
Copy link
Copy Markdown
Contributor Author

Popovers inherit the z-index of the element they are anchored to and sit 2000 levels above that, so if the tour is inside the header, it should be fine. But either way no worries, I'll go ahead and close this PR out for the blockers that Karen mentioned above.

@cee-chen cee-chen closed this Oct 16, 2024
@cee-chen cee-chen deleted the collapsible-nav-beta/z-index branch October 23, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants