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

[UI Framework] [K7] Add KuiCallOut and KuiVerticalRhythm components. #13306

Merged
merged 3 commits into from Aug 3, 2017

Conversation

Projects
None yet
2 participants
@cjcenizal
Copy link
Contributor

commented Aug 3, 2017

@snide The only thing I'm not completely happy with is the vertical rhythm system. The sizes map strangely to our typography. Maybe we should rename them to mirror our typographic terms, e.g. kuiVerticalRhythm--title. Or maybe we should convert them into typographic modifiers, e.g. kuiTitle--verticalRhythm.

image

image

@cjcenizal cjcenizal requested a review from snide Aug 3, 2017

Add KuiCallOut and KuiVerticalRhythm components.
- Add verticalRhythm prop to Typography components.
- Add kuiIcon--basic class.

@cjcenizal cjcenizal force-pushed the cjcenizal:k7/call-out-component branch from 2914894 to d5508dd Aug 3, 2017

@@ -80,7 +80,7 @@ export default () => (

<KuiButton
iconReverse
icon="arrowRight"
iconType="arrowRight"

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

Let's use this pattern of putting "type" in a prop name when we're passing in a string reference to a type.

.kuiCallOut {
padding: $kuiSizeM $kuiSizeL;
border-left: 4px solid $kuiColorPrimary;
background-color: rgba($kuiColorPrimary, 0.1); /* 1 */

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

I think we should change our approach here so that components with modifiers require a modifier to take on an appearance. In other words, we'd remove these default values and add an info definition to $callOutTypes, below (and do something similar with KuiButton and KuiIcon).

I kept running into cascade problems with KuiIcon because of its default fill, and this approach avoids that problem. It also makes it easier to scan the modifiers and know exactly what kind of use cases a component addresses.

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

Again, this is where I why I think modifiers should use &.. Modifiers modify something. Makes perfect sense to use CSS this way.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

It also makes it easier to scan the modifiers and know exactly what kind of use cases a component addresses.

Nesting modifiers to introduce more specificity is definitely an option (which I'm not opposed to), but another benefit of my suggestion is improved maintainability.

export const SIZES = Object.keys(sizeToClassNameMap);
const titleSizeToVerticalRhythmClassNameMap = {
small: 'kuiVerticalRhythm',
large: 'kuiVerticalRhythm--xLarge',

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

Weird mappings.

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

Agree. I don't have a solution, but think this is the best way to "solve" it. Basically that component sizing and CSS global sizing are somewhat distanced to make React components more usable.

@cjcenizal cjcenizal referenced this pull request Aug 3, 2017

Closed

K7 KUI #12807

36 of 51 tasks complete
@snide
Copy link
Contributor

left a comment

Minor changes on the callouts themselves. I can prolly clean it up in a separate PR. Little confused by the margin mixins and the react component. Let's chat it out over zoom.

}

.kuiCallOutHeader__title {
font-weight: $kuiFontWeightMedium;

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

This isn't needed here. We use bolding as a last resort, and don't need it when the color and size (content text should be $fontSizeS // 14px) are different between the title and the content.

image

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

Oh, I actually thought the design had a heavier weight on it. Take a look at these screenshots with the regular weight:

image

image

It looks much lighter than the design to my eyes. To be honest, I think the heaver weight improves readability significantly since the text is the same color as the background, just darker.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

I just checked the color contrast, and the colors do fail at both AA and AAA. I'll make the tint/shade change and take another look.

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

I like the unbolded fwiw. Doesn't feel like it's screaming at me, which alerts can sometimes feel.

@@ -87,3 +88,7 @@ KuiIcon.propTypes = {
size: PropTypes.oneOf(SIZES),
title: PropTypes.string,
};

KuiIcon.defaultProps = {
className: 'kuiIcon--basic',

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

I'm ok with this in the React sense and it totally makes sense. I'm a little worried in the HTML/CSS sense where suddenly default settings of css components are "broken". For example, what's the version of this for .kuiButton? Does that component not usable on its own? Seems so easy to just solve with .kuiIcon.modifier.

Just something to think about.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

Yeah, I was thinking that kuiButton by itself wouldn't be usable without a modifier. Bootstrap is kind of the same way, in that the default button just looks like a weird link.

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

If you think it's the right call I'll go with it! We should probably switch buttons then too.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

Cool thanks man!

.kuiText--small {
@include kuiFontSizeS;
color: $kuiTextColor;
font-weight: $kuiFontWeightRegular;

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

Does font weight need to be on here?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

Good catch!

export const SIZES = Object.keys(sizeToClassNameMap);
const titleSizeToVerticalRhythmClassNameMap = {
small: 'kuiVerticalRhythm',
large: 'kuiVerticalRhythm--xLarge',

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

Agree. I don't have a solution, but think this is the best way to "solve" it. Basically that component sizing and CSS global sizing are somewhat distanced to make React components more usable.

$kuiLineHeightS: $kuiLineHeight * 0.75;
$kuiLineHeightM: $kuiLineHeight;
$kuiLineHeightL: $kuiLineHeight * 1.5;
$kuiLineHeightXL: $kuiLineHeight * 2;

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

I'll need to tweak this stuff separately. Some of it's too fat. I'll do it in another PR.

@@ -0,0 +1,23 @@
.kuiVerticalRhythm {

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

What do these mixins do that the variables themselves don't already provide? When would we ever swap the content inside of them, versus just changing the value of the variable?

As a react component when is it ever a good idea to use <KuiVerticalRhythm size="whatever"> versus just passing it onto the element?

If we did use something like this, it should be called BottomMargin I think? Vertical Rhythm is most often associated with a holistic system to space bodies of text. It's usually a mix of lots of different values, not just spacing, but size, line-height...etc.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 3, 2017

Author Contributor

OK, I'll remove the mixins and components for now, and rename the terminology to bottomMargin.

.kuiCallOut {
padding: $kuiSizeM $kuiSizeL;
border-left: 4px solid $kuiColorPrimary;
background-color: rgba($kuiColorPrimary, 0.1); /* 1 */

This comment has been minimized.

Copy link
@snide

snide Aug 3, 2017

Contributor

I'd prefer if we avoid opacity in places where we can use flat values (tintOrShade works perfectly fine here). I know opacity is easy for theming, but I'd like to avoid the render problems that opacity can aggravate. Anything that we can do to help the slowdowns that K6 suffers from (think of the slowness around resizing the discover page) I think we should try to do. tintOrShade also lets us be more specific and apply separate values.

Opacity is great for animations and shadows. If it's just defining a color, lets use a color.

@cjcenizal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

@snide Could you take another look? I'm going to make the change to remove the default component styles for Button and CallOut in a separate PR. I wasn't sure what to rename the vertical rhythm classes in a way that didn't create confusion with the utility classes we have (e.g. .kui--marginBottom and .kui--marginBottomLarge), so I left them in. Maybe you can take a whack at it after we merge?

I addressed your other feedback and created an additional function for helping us solve color contrast problems. The color contrast level is now AA compliant:

image

image

@snide

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2017

This looks great man!

@snide

snide approved these changes Aug 3, 2017

Copy link
Contributor

left a comment

All good.

@cjcenizal cjcenizal merged commit af2c8b0 into elastic:k7-ui-framework Aug 3, 2017

1 check passed

CLA Commit author has signed the CLA
Details

@cjcenizal cjcenizal deleted the cjcenizal:k7/call-out-component branch Aug 3, 2017

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 16, 2017

[UI Framework] [K7] Add KuiCallOut component. (elastic#13306)
* Add KuiCallOut component.
- Add verticalRhythm prop to Typography components.
- Add kuiIcon--basic class.

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 26, 2017

[UI Framework] [K7] Add KuiCallOut component. (elastic#13306)
* Add KuiCallOut component.
- Add verticalRhythm prop to Typography components.
- Add kuiIcon--basic class.

cjcenizal added a commit that referenced this pull request Sep 19, 2017

[UI Framework] [K7] Add KuiCallOut component. (#13306)
* Add KuiCallOut component.
- Add verticalRhythm prop to Typography components.
- Add kuiIcon--basic class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.