Flyout component #227

snide · 2017-12-20T07:12:41Z

Flyout component. Comes with props for ownFocus and size.

Todo before merge

Turn EuiModalOverlay into EuiOverlayMask so its usage is more generic.
Make these responsive and full width at small res.
Possible add an "X" icon that must always exist in the top right, so the consumer doesn't need to place a button.

Breaking change

EuiModalOverlay is now EuiOverlayMask. It operates the same way. You only need to do a search / replace against the string to apply your patch.

snide · 2017-12-21T00:16:22Z

@cchaos Check this over for design beyond my todos. Only thing I'm wondering... should I put an X icon in the top right corner of these things? ESC closes them, and likely people would use these with action bars that had a close button, but I didn't know if I should add something like that. On one hand it makes sense, but it means i'd likely need to push the content in the header downa bit.

bmcconaghy

LGTM. The escape key did not close the flyout for me in my plugin, but we can address that later.

cchaos · 2017-12-21T15:57:38Z

src/components/flyout/_flyout.scss

+  box-shadow: -$euiSizeXS 0px $euiSizeXS 0px rgba(0,0,0,.05);
+}
+
+$flyoutSizes: (


$euiFlyoutSizes?

cchaos · 2017-12-21T15:59:40Z

src/components/flyout/_flyout_body.scss

+
+  flex-grow: 1;
+  overflow-y: auto;
+  padding: $euiSize;


Can we increase the padding surrounding body, header, and footer at larger widths?

cchaos · 2017-12-21T16:00:04Z

src/components/flyout/_flyout_footer.scss

+  background: $euiColorLightestShade;
+  flex-grow: 0;
+  padding: $euiSize;
+  border-top: $euiBorderThin;


Don't think a border necessary here.

cchaos · 2017-12-21T16:01:16Z

src/components/flyout/_flyout_header.scss

+  flex-grow: 0;
+  padding: $euiSize;
+  padding-bottom: 0;
+  box-shadow: 0 $euiSize $euiSize (-$euiSize / 2) $euiColorEmptyShade;


What's the box-shadow on the header for? I'm not seeing it visually.

It gives a slight fade for the scroll to "shelf" the header.

Ahh youp, I see

cchaos · 2017-12-21T16:05:35Z

src/components/flyout/_flyout.scss

@@ -44,11 +44,3 @@ $flyoutSizes: (
    border-left: none;
  }
 }
-
-@include screenXSmall {


@snide I think you meant to remove the screenSmall selector instead?

cchaos · 2017-12-21T16:08:31Z

src/components/flyout/_flyout.scss

+  border-left: $euiBorderThin;
+  z-index: $euiZModal;
+  background: $euiColorEmptyShade;
+  animation: euiFlyout $euiAnimSpeedNormal $euiAnimSlightResistance;


Feels a bit quick to me for such a large sized component.

I've done these kinds of systems before. We always had them slower, and then eventually sped them because they slowed down the work flow.

I just mean a tad bit slower, not significantly. Also there isn't a closing animation. Is that on purpose?

Our animation rules have generally been to immediately dismiss items on actions (like clicks), but use fades when they are done by the system (like a toast disappearing after a set time). Again, it's just to be snappy.

Yeah I don't think I have a strong feeling either way.

cchaos · 2017-12-21T16:09:11Z

src/components/flyout/_flyout.scss

+    transform: translateX(100%);
+  }
+  100% {
+    opacity: 1;


Because of the size, I think the opacity should get to the full 1 before it's completely in place. Like maybe at 70%?

cchaos · 2017-12-21T16:38:42Z

src/components/flyout/_flyout.scss

+
+@each $name, $size in $flyoutSizes {
+  .euiFlyout--#{$name} {
+    width: $size;


I think the flyouts should also have a minimum width. This will ensure that on smaller screens (before it goes fullscreen) it will be an appropriate size.

cchaos · 2017-12-21T16:44:43Z

On the JS side, I'd recommend that we also (at least eventually) do the following:

Trap scrolling focus so when using the scroll wheel to scroll the flyout, it doesn't also scroll the browser window.
Currently you can have open all the flyouts on the example page at one time. I think clicking on the same triggering element should close it's own flyout, and triggering another flyout to open should close all open flyouts (at least by default). I understand that there might be case to have multiple open at once, but that is rare and should be a special case.

cchaos · 2017-12-21T17:10:00Z

src/components/flyout/_flyout.scss

+  display: flex;
+  flex-direction: column;
+  align-items: strecth;
+  box-shadow: -$euiSizeXS 0px $euiSizeXS 0px rgba(0,0,0,.05);


What do you think about giving it a little more depth via the box-shadow? Something like:

box-shadow: -8px 4px 12px 0px rgba(0, 0, 0, 0.05) in vars of course...

cchaos · 2017-12-21T17:11:38Z

Need to come up with a better dark theme implementation. It's hard to see the boundaries.

cchaos · 2017-12-21T17:34:38Z

src/components/flyout/_flyout.scss

+  animation: euiFlyout $euiAnimSpeedNormal $euiAnimSlightResistance;
+  display: flex;
+  flex-direction: column;
+  align-items: strecth;


spelling error: strecth -> stretch

Responsive handlers

snide · 2017-12-21T19:23:08Z

OK. Got all the big issues and fixed some of the scrolling issues. Thanks to @cchaos for some mobile help. Gonna merge this in. Will make an issue for some other stuff we'd like to change (but aren't critical)

snide added feature request prototype labels Dec 20, 2017

snide added the breaking change label Dec 21, 2017

snide requested review from bmcconaghy and ycombinator December 21, 2017 03:51

snide removed prototype labels Dec 21, 2017

snide force-pushed the feature/flyout branch from 82456cf to 86c5ee4 Compare December 21, 2017 06:55

snide added 6 commits December 20, 2017 22:56

flyout components

a5b3924

snapshots

3909f44

better docs, fix clickable ownfocus state

59d51e5

code comments

a10a5b9

change EuiModalOverlay into EuiOverlayMask

3417a55

responsive flyouts

112dfd7

snide force-pushed the feature/flyout branch from 86c5ee4 to 112dfd7 Compare December 21, 2017 06:56

changelog

35a400e

bmcconaghy approved these changes Dec 21, 2017

View reviewed changes

only show responsive on small screens

222c131

cchaos suggested changes Dec 21, 2017

View reviewed changes

cchaos reviewed Dec 21, 2017

View reviewed changes

snide and others added 5 commits December 21, 2017 09:40

some fixes

2b4dc64

typo

ca313ea

Better responsive sizes for flyouts

befaffb

fix focus state ESC issue

184e751

Merge branch 'feature/flyout' into feature/flyout

850b3f2

snide added 3 commits December 21, 2017 10:28

Merge branch 'feature/flyout' into feature/flyout

a069c5b

Merge pull request #1 from cchaos/feature/flyout

97462b3

Responsive handlers

overlay mask applies hidden to body

eba8fde

snide merged commit 9456e43 into elastic:master Dec 21, 2017

snide deleted the feature/flyout branch December 21, 2017 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flyout component #227

Flyout component #227

snide commented Dec 20, 2017 •

edited

Loading

snide commented Dec 21, 2017

bmcconaghy left a comment

cchaos Dec 21, 2017

cchaos Dec 21, 2017

cchaos Dec 21, 2017

cchaos Dec 21, 2017

snide Dec 21, 2017

cchaos Dec 21, 2017

cchaos Dec 21, 2017

snide Dec 21, 2017

cchaos Dec 21, 2017

snide Dec 21, 2017

cchaos Dec 21, 2017

snide Dec 21, 2017

cchaos Dec 21, 2017

cchaos Dec 21, 2017

cchaos Dec 21, 2017

cchaos commented Dec 21, 2017 •

edited

Loading

cchaos Dec 21, 2017

cchaos commented Dec 21, 2017

cchaos Dec 21, 2017

snide commented Dec 21, 2017

Flyout component #227

Flyout component #227

Conversation

snide commented Dec 20, 2017 • edited Loading

Todo before merge

Breaking change

snide commented Dec 21, 2017

bmcconaghy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

cchaos commented Dec 21, 2017

Choose a reason for hiding this comment

snide commented Dec 21, 2017

snide commented Dec 20, 2017 •

edited

Loading

cchaos commented Dec 21, 2017 •

edited

Loading