Updating Popovers to use Panel Padding vars #229

cchaos · 2017-12-20T16:24:07Z

Takeaway

Popovers with titles no longer require padding to be manually added to the popover's child that follows the title.

Solution

The panel's padding is still applied to the entire popover. Based on the panel padding class applied to the popover, ie .euiPanel--paddingMedium, the title has negative margins to expand to cover that padding amount.

The popover's title also adjusts the horizontal padding to match so that the content and the title align horizontally.

The popover's title always has some padding and only the content's padding is affected by the "none" padding property on the panel.

Design

I, personally, feel that borders around shading are duplicative and noisy, so I also removed the bottom border from the title. I can easily change back if others disagree.

Screenshots

cjcenizal · 2017-12-20T17:05:13Z

@cchaos Could you please add a screenshot of the new popover?

cchaos · 2017-12-20T17:08:56Z

I guess there is still a question about whether all of the title's padding should be affected or just the horizontal padding?

snide

Looks good and I think you're right about the bottom border. I would scan through the src-docs JS files for "padding" though to make sure you got all instances where padding was applied. Noticed one stray one in forms...

cjcenizal

Nice! This is a clever solution. I just had a few suggestions.

cjcenizal · 2017-12-20T17:45:41Z

src/components/panel/_panel.scss

+ *  - Popover
+*/
+$euiPanelPadding: (
+  "Small": $euiSizeS,


I like this pattern! I think we can make it a little easier to grep for the modifier if we use the full modifier name as the key (and use the term "modifier" in the variable names to make it clearer we're not defining an actual padding value):

$euiPanelPaddingModifiers: ( ".euiPanel--paddingSmall": $euiSizeS, ".euiPanel--paddingMedium": $euiSize, ".euiPanel--paddingLarge": $euiSizeL ) !default; .euiPanel { @include euiBottomShadowSmall; background-color: $euiColorEmptyShade; border: $euiBorderThin; border-radius: $euiBorderRadius; flex-grow: 1; @each $modifier, $amount in $euiPanelPaddingModifiers { &#{$modifier} { padding: $amount; } } &.euiPanel--shadow { @include euiBottomShadow; } &.euiPanel--flexGrowZero { flex-grow: 0; } }

What do you think?

I see where you're going with this in that it simplifies the (at)each statement and the popover doesn't need to hard-code the reference to the class. However, I still see too much duplication, ie. .euiPanel. Also I'd like to keep the selectors at the child level so it's obvious they're modifiers. How is this for a compromise:

$euiPanelPaddingModifiers: ( "--paddingSmall": $euiSizeS, "--paddingMedium": $euiSize, "--paddingLarge": $euiSizeL ) !default; .euiPanel { ... @each $modifier, $amount in $euiPanelPaddingModifiers { &.euiPanel#{$modifier} { padding: $amount; } } ... }

too much duplication

What problem are we solving by removing the duplication? For example, does duplication make it more difficult to read? To maintain?

keep selectors at child level so it's obvious they're modifiers

I don't understand, could you clarify? I would think the use of -- makes it clear the class is a modifier because it's a BEM convention.

I think it's a bit of both.

To me, it seems to read easier having .euiPanel duplicated in the actual selector versus in a map. Also, I'm not sure that we want to start going down the road of storing full class names inside of variables. Could be a slippery slope to too many variables.

cjcenizal · 2017-12-20T17:47:27Z

src/components/panel/_panel.scss

@@ -1,3 +1,13 @@
+/** Padding map eferenced in:


Minor nit, but for consistency can we separate the opening comment block token from the first line?

/** * Padding map referenced in: * - Popover */

Also small typo: "eferenced" -> "referenced"

cjcenizal · 2017-12-20T17:49:49Z

src/components/popover/_popover_title.scss

+  // ensure the title expands to cover that padding and
+  // take on the same amount of padding horizontally
+
+  @each $size, $amount in $euiPanelPadding {


$euiPanelPadding is only available here because panel occurs before popover and we're importing components in alphabetical order. I think it would be more explicit and robust if we extract the $euiPanelPaddingModifiers variable somewhere more global to make it clear that it's consumed by multiple modules. What do you think?

Alternatively, if we can I'd rather we don't make more global variables. In the back of our head we should always consider that we're going to pull our components CSS away from each other one day and the less global settings we rely on the better.

We might want to consider directly importing required variables from file to the other rather than just adding it to the global scope, similar to how JS would handle it.

Importing the variables SGTM. What do you mean by pulling our components' CSS away from each other one day?

@cjcenizal If we were building this outside of Kibana's necessities for multi-framework solutions we likely would have built out separate CSS files per component rather than a giant monolith file that needs to be imported (which would be replaced with just a single reset / core file). Importing the one component into another would bring along all the styling you need on a layout per layout basis. One day (not anytime soon) I think we'll end up there.

Would @import '_panel' not duplicate the panel styles?

Also, I thought there was a way to do sass compilation that order of the imports in the manifest doesn't matter?

Looks like it does not duplicate. I'll add the import

cjcenizal · 2017-12-20T17:51:41Z

src/components/popover/_mixins.scss

  padding: $euiSizeM;
+  border-top-left-radius: $euiBorderRadius - 1;
+  border-top-right-radius: $euiBorderRadius - 1;


Can we leave a comment here to explain why we do this?

cjcenizal · 2017-12-20T17:52:15Z

src-docs/src/views/popover/popover_with_title.js

              <EuiText>
                <p>
-                  Popover content
+                  Popover content with default padding


Can we also update the text of the buttons to also reflect that each one demonstrates a different padding size?

We're also missing an example of small padding, aren't we? Maybe we should just create a section dedicated to demonstrating the different padding options.

Yeah I'll just create a new section

cjcenizal · 2017-12-20T18:06:38Z

Re horizontal vs. all padding, I think only horizontal padding should be affected. Adding the vertical padding too seems weird to me.

Buttons now correctly align regardless of if they have icons next to them.

help / error text didn't have line-height.

…c#233)

…tColor (elastic#235) * Rename euiFlexGroup--alignItemsEnd to euiFlexGroup--alignItemsFlexEnd. * Remove support for 'primary' color from EuiTextColor because it looks too much like a link. * Use flex groups in EuiButton examples.

cjcenizal

Looks good! I had a couple last small suggestions.

cjcenizal · 2017-12-21T00:39:32Z

src/components/panel/_panel.scss

-  &.euiPanel--paddingLarge {
-    padding: $euiSizeL;
+  @each $modifier, $amount in $euiPanelPaddingModifiers {
+    &.euiPanel#{$modifier} {


I think I see your point. Could we make one last change and put the -- here instead of in the map? This will be inline with how we're structuring this kind of pattern elsewhere.

cjcenizal · 2017-12-21T00:43:16Z

src/components/popover/_popover_title.scss

@@ -1,3 +1,17 @@
+@import '../panel/panel'; // grab the $euiPanelPaddingModifiers map


Can we move the variables into a panel/variables.scss file? Then it'd be clearer what we're importing, similar to the way we import a different component's mixins in https://github.com/elastic/eui/blob/master/src/components/context_menu/_context_menu_panel.scss#L1

Popovers with titles no longer require padding to be manually added to the popover's child that follows the title.

cchaos added 3 commits December 19, 2017 16:56

working commit

0c4dcf0

Created a panel padding SASS map and using in popover to propagate down

9eb945f

Removed border from title

93a4a0d

cchaos requested review from snide and cjcenizal December 20, 2017 16:24

Reducing spacing

70bd675

snide approved these changes Dec 20, 2017

View reviewed changes

cjcenizal suggested changes Dec 20, 2017

View reviewed changes

cchaos and others added 14 commits December 20, 2017 13:21

Fixed a couple other examples using popover

0131c65

Fix button vertical alignment (elastic#232)

ff4b42f

Buttons now correctly align regardless of if they have icons next to them.

Update CHANGELOG for 0.0.8.

841058d

Updated documentation.

655ae74

0.0.8

b4e45c4

Give help / error text proper line-height (elastic#234)

c94c4c7

help / error text didn't have line-height.

Update release scripts to push to upstream instead of origin. (elasti…

3f866df

…c#233)

Update CHANGELOG for 0.0.9.

46073f0

Updated documentation.

97dd332

0.0.9

b6f8e45

Adding specific example for altering padding with title

7df7069

import panel file directly to make sure we grab the map

38c7e7a

Removing padding props from the title section

21b5568

cjcenizal approved these changes Dec 21, 2017

View reviewed changes

cchaos added 4 commits December 20, 2017 19:53

Last few edits

4a2938e

working commit

174bf8a

Created a panel padding SASS map and using in popover to propagate down

6afd194

Removed border from title

2a2df38

cchaos added 8 commits December 20, 2017 19:54

Reducing spacing

26a173f

Fixed a couple other examples using popover

9cc4c0e

Adding specific example for altering padding with title

a7af4d2

import panel file directly to make sure we grab the map

5047dd5

Removing padding props from the title section

e933daa

Last few edits

5c7d42f

Merge branch 'popovers' of https://github.com/cchaos/eui into popovers

93923a3

changelog

0536246

cchaos merged commit a594b92 into elastic:master Dec 21, 2017

cchaos deleted the popovers branch December 21, 2017 01:03

snide pushed a commit to snide/eui that referenced this pull request Dec 21, 2017

Updating Popovers to use Panel Padding vars (elastic#229)

0493ee6

Popovers with titles no longer require padding to be manually added to the popover's child that follows the title.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating Popovers to use Panel Padding vars #229

Updating Popovers to use Panel Padding vars #229

cchaos commented Dec 20, 2017 •

edited

Loading

cjcenizal commented Dec 20, 2017

cchaos commented Dec 20, 2017

snide left a comment

cjcenizal left a comment

cjcenizal Dec 20, 2017

cchaos Dec 20, 2017 •

edited

Loading

cjcenizal Dec 21, 2017

cchaos Dec 21, 2017

cjcenizal Dec 20, 2017

cjcenizal Dec 20, 2017 •

edited

Loading

snide Dec 20, 2017

cjcenizal Dec 20, 2017

snide Dec 20, 2017

cchaos Dec 20, 2017

cchaos Dec 20, 2017

cchaos Dec 21, 2017

cjcenizal Dec 20, 2017

cjcenizal Dec 20, 2017

cjcenizal Dec 20, 2017

cchaos Dec 20, 2017

cjcenizal commented Dec 20, 2017

cjcenizal left a comment

cjcenizal Dec 21, 2017

cjcenizal Dec 21, 2017

		@@ -1,3 +1,17 @@
		@import '../panel/panel'; // grab the $euiPanelPaddingModifiers map

Updating Popovers to use Panel Padding vars #229

Updating Popovers to use Panel Padding vars #229

Conversation

cchaos commented Dec 20, 2017 • edited Loading

Takeaway

Solution

Design

Screenshots

cjcenizal commented Dec 20, 2017

cchaos commented Dec 20, 2017

snide left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos Dec 20, 2017 • edited Loading

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

cjcenizal Dec 20, 2017 • edited Loading

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

cjcenizal commented Dec 20, 2017

cjcenizal left a 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 20, 2017 •

edited

Loading

cchaos Dec 20, 2017 •

edited

Loading

cjcenizal Dec 20, 2017 •

edited

Loading