Add banner prop to EuiFlyoutBody by andreadelrio · Pull Request #2837 · elastic/eui

andreadelrio · 2020-02-07T07:53:06Z

Summary

In order to build the drilldowns feature, we'd like have a borderless callout at the top of a flyout. For that reason this PR:

Adds a prop called banner to EuiFlyOutBody which takes a node
Adds a prop called borderless to EuiCallOut which removes the usual border from callouts

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

cchaos · 2020-02-07T13:52:21Z

Hmm, part of this PR worries me a bit. Having two different styles of callouts (bordered and not) will cause some major inconsistencies in our applications. Mainly I'm thinking because devs won't necessarily know which one they should choose and pick entirely on aesthetics (which really is the only reason there's a difference).

Looking at the flyout version, it does look nice and I can think of several places that would benefit from this design. Which brings up another thing to consider. Should those other places get updated to reflect this new design (again for consistency)?

So my main question is, does the need for the callout to extend to the sides of the flyout (which is just an aesthetic choice at the moment) outweigh the possibilities of inconsistent callout types throughout the applications?

There's also another approach you could take that would ease the concern of inconsistent callouts, in which you apply a custom className to the provided banner, which in this case would be an EuiCallout and only when that banner className is used in conjunction with .euiCallout do you then remove the border. This would keep stand alone callout's always the same but only in this very specific instance would it remove the border.

If we do move forward with this, something to consider is that the overflow mask in the flyout body is causing the top portion of the callout to fade out as it approaches the header border.

andreadelrio · 2020-02-07T18:21:50Z

@cchaos Thanks for bringing up all these points. I hadn't fully considered the risk of inconsistencies in the use of callouts and I can understand the concern. To answer your question, I think the need for a "full width" callout in a flyout does not warrant the risk of inconsistent callout use. Having said that though I do think we should allow for a "full width" banner in flyouts, it just doesn't have to mean be add a borderless prop to callout.

The flyout with a regular callout as banner looks like this:

I don't think this looks too bad which makes me wonder if we'd need to remove the border at all. We could always leave it up to the consumer to remove the border in a callout when using it in a flyout? Is that bad practice?

cchaos · 2020-02-07T18:39:09Z

I don't think this looks too bad which makes me wonder if we'd need to remove the border at all.

I'll agree that it doesn't look too "bad", but obviously the non-bordered looks a bit better. I'll leave it up to you to decide if you'd want to keep the border in the flyout or not. However, I'd just not add it as a direct option of EuiCallout, but a custom override with classes.

We could always leave it up to the consumer to remove the border in a callout when using it in a flyout? Is that bad practice?

By leave it up to the consumer, do you mean letting them do the overrides or using a prop on EuiCallout? Consumers can override anything they want really, we just don't usually add docs around this specifically. However, adding this override into EUI would be as simple as:

Wrapping the banner element in a div with a custom class like:

<div className="euiFlyout__banner">{banner}</div>`.

In the css, watch for .euiCallOut as being a child of .euiFlyout__banner and then remove the banner if so:

.euiFlyout__banner .euiCallOut {
  border: none;
}

andreadelrio · 2020-02-07T19:42:51Z

@cchaos Agreed about not adding borderless prop to callouts anymore. Yeah I know consumers can override anything, I guess I was wondering if it'll be weird for them to pass a "regular" callout as banner and see it doesn't have a border. But I guess that's not a big deal and I could always make a note in the docs that callouts get their border removed in flyouts.

If we do move forward with this, something to consider is that the overflow mask in the flyout body is causing the top portion of the callout to fade out as it approaches the header border.

About this, I also noticed it and wasn't sure how to handle it. Would we push the banner down a bit,

or remove mask-image?

I like how it looks without mask-image better but don't know how strict we are when it comes to allowing for this type of modifications. Or is there another way to handle it?

cchaos · 2020-02-07T20:55:58Z

I would consider creating an alternate mask-image that just masks the bottom portion and use that.

andreadelrio · 2020-02-11T18:07:39Z

I would consider creating an alternate mask-image that just masks the bottom portion and use that.

@cchaos My plan for this would be to modify euiOverflowShadow to also take a parameter called type which would default to both, but could also take top and bottom. We'd use it with bottom for this case. Let me know if this sounds like a good idea so I can proceed.

andreadelrio · 2020-02-11T22:24:43Z

@cchaos I made the changes we discussed. I also updated the docs to reflect the changes on euiOverflowShadow. I did that for vertical scrolling, not sure if I should do it for horizontal scrolling too as I don't want that portion of the docs to get repetitive

Updated flyout with banner and bottom shadows:

cchaos

@andreadelrio and I talked about not needing the granularity of extra mixins and utility classes that the new params to euiOverflowShadow provides. But she'll throw a link directly to this mixin into the documentation at the very least.

cchaos · 2020-02-11T22:35:15Z

src/components/flyout/flyout_body.tsx

+  HTMLAttributes<HTMLDivElement> &
+    CommonProps & {
+      /**
+       * Use to display a banner at the top of the body. It is suggested to use a borderless `EuiCallOut` for it.


May want to adjust this comment now that there's no such thing as a borderless callout.

cchaos · 2020-02-11T22:35:51Z

src/components/flyout/_flyout_body.scss

+  }
+
+  .euiFlyoutBody__overflow-banner .euiCallOut {
+    border: none;


Add a comment in here why you're altering a different component

cchaos · 2020-02-11T22:44:33Z

src/components/flyout/flyout_body.tsx

 }) => {
  const classes = classNames('euiFlyoutBody', className);
+  const overflowClasses = classNames('euiFlyoutBody__overflow', {
+    'euiFlyoutBody__overflow-banner': banner,


Remember that BEM naming suggests that modifiers should use a -- and you might want to say it hasBanner instead otherwise it sounds like it's trying to target the banner itself.

This class also shouldn't be the one used to target the callout or else any callouts within the rest of children will also get the border removed. So instead, your element would be:

// If banner exists (= true), then spit out the wrapper plus the banner element {banner && <div class="euiFlyoutBody__banner"> {banner} </div> }

cchaos · 2020-02-11T22:47:51Z

src/global_styling/mixins/_shadow.scss

+      transparentize(red, .9) 100%;
+    }
+  } @else {
+    @warn "euiOverflowShadow() expects side to be 'both', 'start' or 'end' but got '#{$side}'";


cchaos · 2020-02-11T22:48:39Z

src/global_styling/mixins/_shadow.scss

+  @if ($side == 'both' or $side == 'start' or $side == 'end') {
+    @if ($side == 'both') {
+      $gradient: transparentize(red, .9) 0%,
+                transparentize(red, 0) $hideHeight,
+                transparentize(red, 0) calc(100% - #{$hideHeight}),
+                transparentize(red, .9) 100%;
+    } @else if ($side == 'start') {
+      $gradient: transparentize(red, .9) 0%,
+      transparentize(red, 0) $hideHeight;
+    } @else {
+      $gradient: transparentize(red, 0) calc(100% - #{$hideHeight}),
+      transparentize(red, .9) 100%;
+    }


If you want to further reduce some repetition here. You could create two top variables of:

$gradientStart: transparentize(red, .9) 0%, transparentize(red, 0) $hideHeight; $gradientEnd: transparentize(red, 0) calc(100% - #{$hideHeight}), transparentize(red, .9) 100%;

And then use those in the if statement:

@if ($side == 'both') { $gradient: $gradientStart, $gradientEnd; } @else if ($side == 'start') { $gradient: $gradientStart; } @else { $gradient: $gradientEnd; }

cchaos

All the changes look good! I would also add a new docs example for the banner prop and a use-case for it.

andreadelrio · 2020-02-13T00:15:56Z

@cchaos I added the example with the banner. I also removed references to onSwitchChange and
isSwitchChecked from the other examples as we are not using them, as far I can tell.

cchaos

🎉 Awesome! Thanks for also cleaning up those unused states and adding a snippet.

[A recent update to EUI](elastic/eui#2837) requires an update to Timeline to restore its original padding, per the before / after screenshots below: ### Before The previous code, which was using the `euiFlyoutBody__overflow` style, has extra padding (before this PR): ![timeline-padding-before-chrome](https://user-images.githubusercontent.com/4459398/76060425-81e37500-5f3e-11ea-8083-a58297afee85.png) ### After Chrome `80.0.3987.132` The new code in this PR uses `euiFlyoutBody__overflowContent`, which restores the Timeline's original padding: ![timeline-paddng-after-chrome-default-theme](https://user-images.githubusercontent.com/4459398/76060436-8b6cdd00-5f3e-11ea-80d6-9ff0e74722c3.png) ![timeline-padding-after-chrome-dark-theme](https://user-images.githubusercontent.com/4459398/76060601-dedf2b00-5f3e-11ea-8a46-1008d7defe09.png) ### After Firefox `73.0.1` ![timeline-padding-after-firefox](https://user-images.githubusercontent.com/4459398/76060692-18b03180-5f3f-11ea-8277-31b6f9dbf211.png) ### After Safari `13.0.5` ![timeline-padding-after-safari](https://user-images.githubusercontent.com/4459398/76060744-38475a00-5f3f-11ea-9b04-cdd45fa41702.png)

## [SIEM] Update Timeline to use the latest euiFlyoutBody style [A recent update to EUI](elastic/eui#2837) requires an update to Timeline to restore its original padding, per the before / after screenshots below: ### Before The previous code, which was using the `euiFlyoutBody__overflow` style, has extra padding (before this PR): ![timeline-padding-before-chrome](https://user-images.githubusercontent.com/4459398/76060425-81e37500-5f3e-11ea-8083-a58297afee85.png) ### After Chrome `80.0.3987.132` The new code in this PR uses `euiFlyoutBody__overflowContent`, which restores the Timeline's original padding: ![timeline-paddng-after-chrome-default-theme](https://user-images.githubusercontent.com/4459398/76060436-8b6cdd00-5f3e-11ea-80d6-9ff0e74722c3.png) ![timeline-padding-after-chrome-dark-theme](https://user-images.githubusercontent.com/4459398/76060601-dedf2b00-5f3e-11ea-8a46-1008d7defe09.png) ### After Firefox `73.0.1` ![timeline-padding-after-firefox](https://user-images.githubusercontent.com/4459398/76060692-18b03180-5f3f-11ea-8277-31b6f9dbf211.png) ### After Safari `13.0.5` ![timeline-padding-after-safari](https://user-images.githubusercontent.com/4459398/76060744-38475a00-5f3f-11ea-9b04-cdd45fa41702.png)

…#59561) ## [SIEM] Update Timeline to use the latest euiFlyoutBody style [A recent update to EUI](elastic/eui#2837) requires an update to Timeline to restore its original padding, per the before / after screenshots below: ### Before The previous code, which was using the `euiFlyoutBody__overflow` style, has extra padding (before this PR): ![timeline-padding-before-chrome](https://user-images.githubusercontent.com/4459398/76060425-81e37500-5f3e-11ea-8083-a58297afee85.png) ### After Chrome `80.0.3987.132` The new code in this PR uses `euiFlyoutBody__overflowContent`, which restores the Timeline's original padding: ![timeline-paddng-after-chrome-default-theme](https://user-images.githubusercontent.com/4459398/76060436-8b6cdd00-5f3e-11ea-80d6-9ff0e74722c3.png) ![timeline-padding-after-chrome-dark-theme](https://user-images.githubusercontent.com/4459398/76060601-dedf2b00-5f3e-11ea-8a46-1008d7defe09.png) ### After Firefox `73.0.1` ![timeline-padding-after-firefox](https://user-images.githubusercontent.com/4459398/76060692-18b03180-5f3f-11ea-8277-31b6f9dbf211.png) ### After Safari `13.0.5` ![timeline-padding-after-safari](https://user-images.githubusercontent.com/4459398/76060744-38475a00-5f3f-11ea-9b04-cdd45fa41702.png)

andreadelrio added 2 commits February 6, 2020 23:34

added banner to EuiFlyout

182b066

Merge remote-tracking branch 'upstream/master' into neutral-callout

82e51c0

andreadelrio force-pushed the neutral-callout branch from 9378870 to 5fda399 Compare February 7, 2020 08:04

cl

f1a10f7

andreadelrio force-pushed the neutral-callout branch from 5fda399 to f1a10f7 Compare February 7, 2020 08:17

andreadelrio requested a review from cchaos February 7, 2020 08:19

andreadelrio added 5 commits February 11, 2020 14:00

updated mixin

f00c117

updated mixin

623892e

remove borderless example

001b695

remove border from callout when in a flyout

d299e94

remove extra line

3251a63

cchaos reviewed Feb 11, 2020

View reviewed changes

pr feedback

3571111

andreadelrio force-pushed the neutral-callout branch from f048ae0 to 587392c Compare February 12, 2020 19:33

update cl again

5c1d80d

andreadelrio force-pushed the neutral-callout branch from 587392c to 5c1d80d Compare February 12, 2020 19:36

andreadelrio requested a review from cchaos February 12, 2020 19:43

cchaos reviewed Feb 12, 2020

View reviewed changes

andreadelrio added 5 commits February 12, 2020 15:54

added flyout with banner example

08c631f

Merge remote-tracking branch 'upstream/master' into neutral-callout

3cca7bf

clean up new example

1b50635

more cleanup

87f95b9

fix reference to source

ece3639

cchaos approved these changes Feb 13, 2020

View reviewed changes

andreadelrio merged commit 11bc776 into elastic:master Feb 13, 2020

streamich mentioned this pull request Feb 21, 2020

<DrilldownHelloBar> component elastic/kibana#55311

Closed

3 tasks

chandlerprall mentioned this pull request Mar 3, 2020

Upgrade EUI to v20.0.2 elastic/kibana#59199

Merged

2 tasks

andrew-goldstein mentioned this pull request Mar 6, 2020

[SIEM] Update Timeline to use the latest euiFlyoutBody style elastic/kibana#59524

Merged

Conversation

andreadelrio commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

cchaos commented Feb 7, 2020

Uh oh!

andreadelrio commented Feb 7, 2020

Uh oh!

cchaos commented Feb 7, 2020

Uh oh!

andreadelrio commented Feb 7, 2020

Uh oh!

cchaos commented Feb 7, 2020

Uh oh!

andreadelrio commented Feb 11, 2020

Uh oh!

andreadelrio commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Feb 13, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andreadelrio commented Feb 7, 2020 •

edited

Loading

andreadelrio commented Feb 11, 2020 •

edited

Loading