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

[EuiHeader] Stacked example #3538

Merged
merged 13 commits into from
Jun 4, 2020
Merged

[EuiHeader] Stacked example #3538

merged 13 commits into from
Jun 4, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 2, 2020

Part 2/3 for next iteration of K8 support; Fixes #3504

This PR is mostly a docs example as it does not add any new props. However it does contain a breaking change to the way the position="fixed" version handles adding body padding. Spoiler, it no longer does.

Rendered example:

Screen Shot 2020-06-02 at 14 23 15 PM

Dark mode theme="dark" now renders slightly darker to create visual separation when they're stacked.

Screen Shot 2020-06-02 at 14 37 09 PM

Breaking change

Before, when a consumer added position="fixed" it would add the euiBody--headerIsFixed body class and apply top padding styles. However, some consumers may want this and some may not, but there's no configuration option. And with the allowed stacked headers, EUI cannot know how many headers they are stacking.

So now it's on the now consumer to apply the appropriate padding and EUI provides both variables and mixins to help. The fixed header example was updated to describe this in description and snippet.

Screen Shot 2020-06-02 at 14 43 25 PM

The stacked example provides a SASS snippet as well.

Screen Shot 2020-06-02 at 14 43 33 PM

Known visual problem

In Amsterdam, the new flyout shadows extend top and bottom instead of just to the left of flyouts, so they will overlap any fixed headers. We can’t simply use z-index because of nesting (flyout lives inside of header DOM) and we’d still have issues if they used portals because the flyout would always come after. We may need a specific shadow for flyouts or use clip-mask.

image

I've added to list of Amsterdam bugs and will be fixed in a later PR.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • Added documentation 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

@kibanamachine
Copy link

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

@cchaos cchaos marked this pull request as ready for review June 2, 2020 22:03
@kibanamachine
Copy link

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

@snide snide added the priority label Jun 3, 2020
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.

These are small quibbles and shouldn't be considered blockers for merge. I REALLY like the way you're demoing the fixed headers and my immediately want was to see this with the collapsable nav so we can see the fully completed death start.

I'd err towards the final patterns in your examples

It's good to show the flexibility of these components, but you might want to show the end result patterns as the examples, especially in the stacked view.

image

src-docs/src/components/guide_components.scss Show resolved Hide resolved
src-docs/src/components/guide_components.scss Show resolved Hide resolved
src-docs/src/views/header/header_example.js Outdated Show resolved Hide resolved
src-docs/src/views/header/header_example.js Outdated Show resolved Hide resolved
src-docs/src/views/header/header_example.js Outdated Show resolved Hide resolved
src-docs/src/views/header/header_example.js Show resolved Hide resolved
@kibanamachine
Copy link

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

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. Reasoning for holding for changes till the next PR makes sense to me.

@kibanamachine
Copy link

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

@snide
Copy link
Contributor

snide commented Jun 3, 2020

@cchaos Does this PR fix #3504?

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Missing a word in the changelog, but otherwise LGTM!

CHANGELOG.md Outdated
@@ -35,6 +35,10 @@
- Removed borders from `EuiFlyout` and `EuiPopover` ([#3477](https://github.com/elastic/eui/pull/3477))
- Updated `EuiHeader` and components ([#3524](https://github.com/elastic/eui/pull/3524))

**Breaking changes**

- A fixed `EuiHeader` no longer automatically padding directly to the `<body>` element ([#3538](https://github.com/elastic/eui/pull/3538))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- A fixed `EuiHeader` no longer automatically padding directly to the `<body>` element ([#3538](https://github.com/elastic/eui/pull/3538))
- A fixed `EuiHeader` no longer automatically adds padding directly to the `<body>` element ([#3538](https://github.com/elastic/eui/pull/3538))

@cchaos
Copy link
Contributor Author

cchaos commented Jun 3, 2020

@cchaos Does this PR fix #3504?

Cool, yeah I think so. It doesn't address any modal content, but I think that's just piecing other EUI components together. 🎉

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally and looked at the code.

@cchaos cchaos merged commit c134b58 into elastic:master Jun 4, 2020
@cchaos cchaos deleted the k8/double_header branch June 4, 2020 14:42
@timroes
Copy link
Contributor

timroes commented Jun 7, 2020

@cchaos I think this PR sneaked in a couple of issues into the documentation.

You will then need to apply your own padding to this body class to afford for the header height. EUI supplies a helper mixin that also accounts for this height in flyouts and the collapsible nav. Simply add @mixin euiHeaderAffordForFixed; anywhere in your SASS.

I think this should be @include euiHeaderAffordForFixed; and not @mixin if you want to include it. Also it seems, you MUST add it into a body rule and not anywhere in your SASS, since the style of that mixin currently uses &.euiBody--headerIsFixed to set the padding.

Also I am having problems actually applying this without specifying the value (seems the default is not working):

@import '@elastic/eui/src/global_styling/variables';
@import '@elastic/eui/src/global_styling/mixins';

body {
  @include euiHeaderAffordForFixed;
}

results in the error:

SassError: Undefined variable: "$euiHeaderHeightCompensation".
        on line 1 of node_modules/@elastic/eui/src/global_styling/mixins/_header.scss, in mixin `euiHeaderAffordForFixed`
        from line 5 of src/app/header.scss
>> in euiHeaderAffordForFixed($headerHeight: $euiHeaderHeightCompensation) {

Do I need to import a specific other sass file to have this variable working or could this be an issue? Looking for the variable looks like it's only set in any of the global files (that we advice in the documentation to include) for the amsterdam theme, but not any other theme (and only local in the header component, which I assume another project should not directly import)?

@cchaos
Copy link
Contributor Author

cchaos commented Jun 10, 2020

Good catch @timroes , thanks for this. I'll create an issue based on that and get them fixed.

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

Successfully merging this pull request may close these issues.

[WIP] Global header: deployment switcher
7 participants