Skip to content
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

(Trying to) Fix z-indexes with header and flyouts & adding more EuiHeader docs #3655

Merged
merged 23 commits into from
Jul 1, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 24, 2020

Z-index

This diagram shows the extend to which we needed more stringent attention to our z-index levels (and yet simplify at the same time).

Screen Shot 2020-06-24 at 16 28 51 PM

EuiOverlayMask

This is the main component that needed more flexibility to change it's z-index. Consumers can always pass in a custom class or style prop to override the default z-index, but I also wanted to provide a quick way for flyouts associated with the header to get the correct z-index.

So I added the prop headerZindexLocation?: 'above' | 'below'. By default all masks will stay "above" the header, but by passing "below", we adjust the z-index to be 1 level below the header: z-index: $euiZHeader - 1.

Flyouts & EuiCollapsibleNav

All flyouts with ownFocus that include the overlay mask now apply the headerZindexLocation="below" prop by default. Though consumers can still adjust this with the new maskProps prop.

In SASS I added a $euiZFlyout var set to 3000 (above header, but below mask default). So if consumers do change the headerZindexLocation prop, they will also need to manually change the z-index of flyouts, but that is a very rare need.

EuiHeader fixed

I removed any additional z-indexing for when the head is fixed since it's no longer necessary to bump it up.

Docs

I added a few new docs sections as well for some of these new props/patterns.

Overlay mask with header

Screen Shot 2020-06-24 at 16 48 25 PM

Portal content in the header

I changed the EuiHeaderAlert example to talk more specifically about using popover vs flyout. In a follow-up I will create a new page for EuiHEaderAlert specifically which is lacking in docs at the moment.

Screen Shot 2020-06-24 at 16 50 43 PM

The Elastic navigation pattern

At the bottom of the Header page I've added a "final" pattern with all the bits needed to display our Elastic global headers. The contents of the flyouts and popovers redirect users to that specific component as the upkeep to keep them in sync but separate is too much. I also made sure that the CodeSandbox link would render and optimized for it though CSB is broken again because of icons.

Screen Shot 2020-06-24 at 16 53 12 PM

Screen Shot 2020-06-24 at 12 36 13 PM

Other fixes

  • Moved the clip-path of the flyout from being Amsterdam specific to being applied to all themes
  • Fixed Amsterdam header breadcrumb border-radius and overall height
  • Fixed EuiHeaderLink item margins to account for any type of children
  • Fixed EuiBreadcrumb support for onClick
  • Fixed EuiBadge support for any valid chroma.js color string. While it always displayed correctly, the console warning wasn't.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Safari and Firefox and Kibana
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately 🎉 😆
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos
Copy link
Contributor Author

cchaos commented Jun 24, 2020

This should probably be tested locally against Kibana before merging as well. But is at least ready for a review from the code and docs side.

@cchaos cchaos requested review from snide and thompsongl June 24, 2020 20:57
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3655/

@chandlerprall
Copy link
Contributor

What are you thoughts on creating a specific z-index concerns/customizations/practices page in the docs?

@cchaos
Copy link
Contributor Author

cchaos commented Jun 29, 2020

Probably a good idea at some point but don't want to hold up Kibana updates for docs.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Code LGTM. Just a couple docs edits

src-docs/src/views/header/header_example.js Outdated Show resolved Hide resolved
src-docs/src/views/overlay_mask/overlay_mask_example.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3655/

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

LGTM. I clicked around in the usual places in Kibana / EUI. One small opinion, but feel free to merge! Nice work.

@@ -50,6 +57,7 @@ export const EuiOverlayMask: FunctionComponent<EuiOverlayMaskProps> = ({
className,
children,
onClick,
headerAdjacent = 'above',
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like this propname, and kind of don't :D. Adjacent doesn't really grok with the normal language, which I think would be something like sibling. headerZindexLocation might be a little more obvious to someone looking for this or trying to figure out what it does.

It's a nit, because the doc describes it well enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I struggled with the name on that one. I'll change.

@@ -27,6 +27,7 @@ $euiZLevel9: 9000;
$euiZContent: $euiZLevel0;
$euiZHeader: $euiZLevel1;
$euiZContentMenu: $euiZLevel2;
$euiZFlyout: $euiZLevel3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Were we really this lucky with the numbering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently! 😏

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3655/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3655/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants