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.

Sign up for GitHub

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

Updating Popovers to use Panel Padding vars #229

Merged
merged 30 commits into from
Dec 21, 2017
Merged

Updating Popovers to use Panel Padding vars #229

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
0c4dcf0
working commit
cchaos Dec 19, 2017
9eb945f
Created a panel padding SASS map and using in popover to propagate down
cchaos Dec 20, 2017
93a4a0d
Removed border from title
cchaos Dec 20, 2017
70bd675
Reducing spacing
cchaos Dec 20, 2017
0131c65
Fixed a couple other examples using popover
cchaos Dec 20, 2017
ff4b42f
Fix button vertical alignment (#232)
snide Dec 20, 2017
841058d
Update CHANGELOG for 0.0.8.
cjcenizal Dec 20, 2017
655ae74
Updated documentation.
cjcenizal Dec 20, 2017
b4e45c4
0.0.8
cjcenizal Dec 20, 2017
c94c4c7
Give help / error text proper line-height (#234)
snide Dec 20, 2017
3f866df
Update release scripts to push to upstream instead of origin. (#233)
cjcenizal Dec 20, 2017
bb89192
Rename EuiFlexGroup classes to reflect CSS values, remove primary Tex…
cjcenizal Dec 20, 2017
46073f0
Update CHANGELOG for 0.0.9.
cjcenizal Dec 20, 2017
97dd332
Updated documentation.
cjcenizal Dec 20, 2017
b6f8e45
0.0.9
cjcenizal Dec 20, 2017
7df7069
Adding specific example for altering padding with title
cchaos Dec 21, 2017
38c7e7a
import panel file directly to make sure we grab the map
cchaos Dec 21, 2017
21b5568
Removing padding props from the title section
cchaos Dec 21, 2017
4a2938e
Last few edits
cchaos Dec 21, 2017
174bf8a
working commit
cchaos Dec 19, 2017
6afd194
Created a panel padding SASS map and using in popover to propagate down
cchaos Dec 20, 2017
2a2df38
Removed border from title
cchaos Dec 20, 2017
26a173f
Reducing spacing
cchaos Dec 20, 2017
9cc4c0e
Fixed a couple other examples using popover
cchaos Dec 20, 2017
a7af4d2
Adding specific example for altering padding with title
cchaos Dec 21, 2017
5047dd5
import panel file directly to make sure we grab the map
cchaos Dec 21, 2017
e933daa
Removing padding props from the title section
cchaos Dec 21, 2017
5c7d42f
Last few edits
cchaos Dec 21, 2017
93923a3
Merge branch 'popovers' of https://github.com/cchaos/eui into popovers
cchaos Dec 21, 2017
0536246
changelog
cchaos Dec 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions src-docs/src/views/popover/popover_with_title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,12 @@ export default class extends Component {
closePopover={this.closePopover1.bind(this)}
anchorPosition="downCenter"
withTitle
panelPaddingSize="none"
>
<EuiPopoverTitle>Hello, I&rsquo;m a popover title</EuiPopoverTitle>
<div style={{ width: '300px', padding: 16 }}>
<div style={{ width: '300px' }}>
<EuiText>
<p>
Popover content
Popover content with default padding
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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'll just create a new section

</p>
</EuiText>
</div>
Expand All @@ -98,13 +97,13 @@ export default class extends Component {
closePopover={this.closePopover2.bind(this)}
anchorPosition="upCenter"
withTitle
panelPaddingSize="none"
panelPaddingSize="l"
>
<EuiPopoverTitle>Hello, I&rsquo;m a popover title</EuiPopoverTitle>
<div style={{ width: '300px', padding: 16 }}>
<div style={{ width: '300px' }}>
<EuiText>
<p>
Popover content
Popover content with large padding
</p>
</EuiText>
</div>
Expand All @@ -127,10 +126,10 @@ export default class extends Component {
panelPaddingSize="none"
>
<EuiPopoverTitle>Hello, I&rsquo;m a popover title</EuiPopoverTitle>
<div style={{ width: '300px', padding: 16 }}>
<div style={{ width: '300px' }}>
<EuiText>
<p>
Popover content
Popover content with no padding
</p>
</EuiText>
</div>
Expand Down
24 changes: 14 additions & 10 deletions src/components/panel/_panel.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
/** Padding map eferenced in:
Copy link
Contributor

Choose a reason for hiding this comment

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

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"

* - Popover
*/
$euiPanelPadding: (
"Small": $euiSizeS,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@cchaos cchaos Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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;
    }
  }

...

}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"Medium": $euiSize,
"Large": $euiSizeL
) !default;


.euiPanel {
@include euiBottomShadowSmall;

Expand All @@ -6,16 +16,10 @@
border-radius: $euiBorderRadius;
flex-grow: 1;

&.euiPanel--paddingSmall {
padding: $euiSizeS;
}

&.euiPanel--paddingMedium {
padding: $euiSize;
}

&.euiPanel--paddingLarge {
padding: $euiSizeL;
@each $size, $amount in $euiPanelPadding {
&.euiPanel--padding#{$size} {
padding: $amount;
}
}

&.euiPanel--shadow {
Expand Down
3 changes: 2 additions & 1 deletion src/components/popover/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
@mixin euiPopoverTitle {
background-color: $euiColorLightestShade;
border-bottom: $euiBorderThin;
padding: $euiSizeM;
border-top-left-radius: $euiBorderRadius - 1;
border-top-right-radius: $euiBorderRadius - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
14 changes: 13 additions & 1 deletion src/components/popover/_popover_title.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
.euiPopoverTitle {
@include euiPopoverTitle;
}

// If the popover's containing panel has padding applied,
// ensure the title expands to cover that padding and
// take on the same amount of padding horizontally

@each $size, $amount in $euiPanelPadding {
Copy link
Contributor

@cjcenizal cjcenizal Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

$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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.euiPopover__panel.euiPanel--padding#{$size} & {
padding: $euiSizeM $amount;
margin: ($amount * -1) ($amount * -1) $amount;
}
}

}