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

[UI Framework] Reactify kuiCard and related CSS components #12197

Merged
merged 7 commits into from
Jun 13, 2017
Merged

[UI Framework] Reactify kuiCard and related CSS components #12197

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
db71202
issue/12170 [UI Framework] Reactify the kuiCard and related CSS compo…
PopradiArpad Jun 5, 2017
84eecae
adapt doc views of card to simpler CSS syntax, fix property name unit…
PopradiArpad Jun 6, 2017
e5b2e1f
add defaultProps to KuiCardGroup, add hint to example code
PopradiArpad Jun 7, 2017
c36765d
use 3 cards in the card group example to see the layout better
PopradiArpad Jun 7, 2017
23acc84
remove KuiCardGroupContainer: not needed with the right KuiCardGroup …
PopradiArpad Jun 7, 2017
d8373b6
fix indentation in ui_framework/doc_site/src/views/card/card_group.js
PopradiArpad Jun 9, 2017
6c3e802
fix typo in comment
PopradiArpad Jun 9, 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
11 changes: 0 additions & 11 deletions ui_framework/components/card/__snapshots__/card_group_container.test.js.snap
View file
Open in desktop

This file was deleted.

24 changes: 13 additions & 11 deletions ui_framework/components/card/_card_group.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,18 @@
/**
* 1. Use this container to offset the spacing between wrapped cards.
*/
.kuiCardGroupContainer {
margin: -$cardSpacing; /* 1 */
}

/**
* 1. Wrap cards when necessary.
* 2. Offset the spacing between wrapped cards.
*/
.kuiCardGroup {
display: flex;
flex-wrap: wrap; /* 1 */
margin: -$cardSpacing; /* 2 */

/**
* 1. Use the defined width of the card to determine when to wrap.
* 2. Use an even margin all around the card so that the spacing is still even when wrapped.
*/
.kuiCard {
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, this violates the open/closed principle by modifying the .kuiCard block based on its location inside the .kuiCardGroup block. I know several other frameworks break this rule by implementing such groups in exactly this way. A way to fix this would be to not nest the selectors and create a mix of .kuiCard and .kuiCardGroup__card on the individual card elements.

References:

@cjcenizal, it looks like this is consistent with how similar group blocks are implemented in the kui framework (e.g. .kuiButtonGroup). So this is not specific to this PR. Do we accept this BEM violation for these cases?

Copy link
Contributor Author

@PopradiArpad PopradiArpad Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@weltenwort, in general you are right, changing the properties through nesting is a secure way into the CSS Hell, but in these cases we have an extra semantic information:
XGroup is a group of X so we are speaking about Xs.
The pro is simpler syntax for the user of the X React component: No extra property is needed on X depending on its location (is X part of a group of Xs or not?) and not to expose CSS, which is a main advantage of the framework.
The con is the loosening of the clear openclosed-principle.
But this loosening is context bound: only within a group of X. It could be considered as an extension of the principle instead of its violation.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're absolutely right... this is a dilemma I've encountered several times. For context, this component was originally implemented the way you suggest, and I only recently changed it to use the current pattern (https://github.com/elastic/kibana/pull/12183/files#diff-5b434436eea8c6ef3f9c1f64780bb791L3 for reference).

There are two problems I've encountered when strictly applying BEM in this kind of situation:

  1. Mixing classes is sometimes required on many elements of a single component, e.g. in the link I shared you not only need .kuiCard mixed with .kuiCardGroup__card but you also need .kuiCard__description mixed with .kuiCardGroup__cardDescription. From the point of view of someone applying the classes, this "interface" can be complex and confusing.
  2. Converting these components to React then requires passing in these classes manually, as you can see in the first commit of this PR. An alternative would be to create a wrapper component which applies this class for you. But again, I feel like both cases are added complexity in the interface.

To me, it comes down to a trade-off: sacrificing BEM in exchange for a more ergonomic interface. Personally, I think we should try to stick to BEM as much as possible, until we discover some pain in the interface. What are your thoughts on this?

Also, do you think we should document your links and these lines of reasoning in a style guide?

Copy link
Contributor Author

@PopradiArpad PopradiArpad Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

BEM is good and known, that's why I can read your SCSS
and diverging from a standard needs explanation.
The css_style_guide.md looks to me the right place for that.

Copy link
Member

Choose a reason for hiding this comment

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

@cjcenizal, thanks for the historic references. I tend to agree with you and @PopradiArpad that this looks like a good exemption from the rule. We should definitely document that somewhere. I will create a PR to amend the css style guide.

Copy link
Member

Choose a reason for hiding this comment

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

for the record, the aforementioned PR is #12276

flex: 1 1 0; /* 1 */
flex: 1 1 auto; /* 1 */
margin: $cardSpacing; /* 2 */

.kuiCard__description {
Expand All @@ -28,17 +23,24 @@

/**
* 1. There's no way to make this look good when wrapped.
* 2. Undo the default styles.
*/
.kuiCardGroup--united {
flex-wrap: nowrap; /* 1 */
border: 1px solid $cardBorderColor;
border-radius: $globalBorderRadius;
overflow: hidden;
margin: 0; /* 2 */

/**
* 1. Force all cards to be the same size.
* 2. Undo the default styles.
*/
.kuiCard {
border: none;
border-radius: 0;
margin: 0;
flex-basis: 0; /* 1 */
border: none; /* 2 */
border-radius: 0; /* 2 */
margin: 0; /* 2 */
}

.kuiCard + .kuiCard {
Expand Down
5 changes: 5 additions & 0 deletions ui_framework/components/card/card_group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ export const KuiCardGroup = ({ children, className, isUnited, ...rest }) => {
const classes = classNames('kuiCardGroup', className, { 'kuiCardGroup--united': isUnited });
return <div className={classes} {...rest}>{children}</div>;
};

KuiCardGroup.defaultProps = {
isUnited: false
};

KuiCardGroup.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
Expand Down
13 changes: 0 additions & 13 deletions ui_framework/components/card/card_group_container.js
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions ui_framework/components/card/card_group_container.test.js
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion ui_framework/components/card/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@ export { KuiCardDescriptionText } from './card_description_text';
export { KuiCardDescriptionTitle } from './card_description_title';
export { KuiCardDescription } from './card_description';
export { KuiCardFooter } from './card_footer';
export { KuiCardGroupContainer } from './card_group_container';
export { KuiCardGroup } from './card_group';
1 change: 0 additions & 1 deletion ui_framework/components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export {
KuiCardDescriptionTitle,
KuiCardDescription,
KuiCardFooter,
KuiCardGroupContainer,
KuiCardGroup,
} from './card';

Expand Down
34 changes: 22 additions & 12 deletions ui_framework/dist/ui_framework.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,15 +562,9 @@ body {
font-size: 14px;
padding: 0 15px 20px; }

/**
* 1. Use this container to offset the spacing between wrapped cards.
*/
.kuiCardGroupContainer {
margin: -15px;
/* 1 */ }

/**
* 1. Wrap cards when necessary.
* 2. Offset the spacing between wrapped cards.
*/
.kuiCardGroup {
display: -webkit-box;
Expand All @@ -581,15 +575,17 @@ body {
-ms-flex-wrap: wrap;
flex-wrap: wrap;
/* 1 */
margin: -15px;
/* 2 */
/**
* 1. Use the defined width of the card to determine when to wrap.
* 2. Use an even margin all around the card so that the spacing is still even when wrapped.
*/ }
.kuiCardGroup .kuiCard {
-webkit-box-flex: 1;
-webkit-flex: 1 1 0;
-ms-flex: 1 1 0px;
flex: 1 1 0;
-webkit-flex: 1 1 auto;
-ms-flex: 1 1 auto;
flex: 1 1 auto;
/* 1 */
margin: 15px;
/* 2 */ }
Expand All @@ -601,6 +597,7 @@ body {

/**
* 1. There's no way to make this look good when wrapped.
* 2. Undo the default styles.
*/
.kuiCardGroup--united {
-webkit-flex-wrap: nowrap;
Expand All @@ -609,11 +606,24 @@ body {
/* 1 */
border: 1px solid #E0E0E0;
border-radius: 4px;
overflow: hidden; }
overflow: hidden;
margin: 0;
/* 2 */
/**
* 1. Force all cards to be the same size.
* 2. Undo the default styles.
*/ }
.kuiCardGroup--united .kuiCard {
-webkit-flex-basis: 0;
-ms-flex-preferred-size: 0;
flex-basis: 0;
/* 1 */
border: none;
/* 2 */
border-radius: 0;
margin: 0; }
/* 2 */
margin: 0;
/* 2 */ }
.kuiCardGroup--united .kuiCard + .kuiCard {
border-left: 1px solid #E0E0E0; }

Expand Down
139 changes: 92 additions & 47 deletions ui_framework/doc_site/src/views/card/card_group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import React from 'react';

import {
KuiCardGroupContainer,
KuiCardGroup,
KuiCard,
KuiCardDescription,
Expand All @@ -12,58 +11,82 @@ import {
} from '../../../../components';

export default () => {
/**
* These styles are just for demonstration purposes. It is recommended use
Copy link
Member

Choose a reason for hiding this comment

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

Oops, had a typo in the proposed text 😊 better: It is recommended to use...

Copy link
Contributor Author

@PopradiArpad PopradiArpad Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

... and I haven't caught that.... ooops :)
Ok

* properly named classes to set the width in production code.
*/
const cardStyle = {
Copy link
Member

Choose a reason for hiding this comment

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

How about adding a comment like the following here?

/**
 * These styles are just for demonstration purposes. It is recommended use
 * properly named classes to set the width in production code.
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make thinking the fast copy-pasters :)
Good.

width: '400px'
};

return (
<div>
<KuiCardGroupContainer>
<KuiCardGroup>
<KuiCard style={cardStyle}>
<KuiCardDescription>
<KuiCardDescriptionTitle>
Get a banana
</KuiCardDescriptionTitle>

<KuiCardDescriptionText>
Bananas are yellow, fit easily in the hand, and have a lot of potassium or something.
</KuiCardDescriptionText>
</KuiCardDescription>

<KuiCardFooter>
<KuiLinkButton
buttonType="basic"
href="#"
>
Banana!
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>

<KuiCard style={cardStyle}>
<KuiCardDescription>
<KuiCardDescriptionTitle>
Get a pteradactyl
</KuiCardDescriptionTitle>

<KuiCardDescriptionText>
Pteradactyls can fly, like to squawk all the time, and are difficult to spell correctly.
</KuiCardDescriptionText>
</KuiCardDescription>

<KuiCardFooter>
<KuiLinkButton
buttonType="primary"
href="https://www.elastic.co/subscriptions/xpack"
target="_blank"
>
Pteradactyl!
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>
</KuiCardGroup>
</KuiCardGroupContainer>
<KuiCardGroup>
<KuiCard style={cardStyle}>
<KuiCardDescription>
<KuiCardDescriptionTitle>
Get a banana
</KuiCardDescriptionTitle>

<KuiCardDescriptionText>
Bananas are yellow, fit easily in the hand, and have a lot of potassium or something.
</KuiCardDescriptionText>
</KuiCardDescription>

<KuiCardFooter>
<KuiLinkButton
buttonType="basic"
href="#"
>
Banana!
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>

<KuiCard style={cardStyle}>
<KuiCardDescription>
<KuiCardDescriptionTitle>
Get a pteradactyl
</KuiCardDescriptionTitle>

<KuiCardDescriptionText>
Pteradactyls can fly, like to squawk all the time, and are difficult to spell correctly.
</KuiCardDescriptionText>
</KuiCardDescription>

<KuiCardFooter>
<KuiLinkButton
buttonType="primary"
href="https://www.elastic.co/subscriptions/xpack"
target="_blank"
>
Pteradactyl!
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>

<KuiCard style={cardStyle}>
<KuiCardDescription>
<KuiCardDescriptionTitle>
Get a magnolia tree
</KuiCardDescriptionTitle>

<KuiCardDescriptionText>
Magnolia trees have broad, waxy leaves which they shed year-round.
</KuiCardDescriptionText>
</KuiCardDescription>

<KuiCardFooter>
<KuiLinkButton
buttonType="basic"
href="https://www.elastic.co/subscriptions/xpack"
target="_blank"
>
Magnolia!
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>
</KuiCardGroup>

<br className="guideBreak"/>

Expand Down Expand Up @@ -110,6 +133,28 @@ export default () => {
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>

<KuiCard style={cardStyle}>
<KuiCardDescription>
<KuiCardDescriptionTitle>
Get a magnolia tree
</KuiCardDescriptionTitle>

<KuiCardDescriptionText>
Magnolia trees have broad, waxy leaves which they shed year-round.
</KuiCardDescriptionText>
</KuiCardDescription>

<KuiCardFooter>
<KuiLinkButton
buttonType="basic"
href="https://www.elastic.co/subscriptions/xpack"
target="_blank"
>
Magnolia!
</KuiLinkButton>
</KuiCardFooter>
</KuiCard>
</KuiCardGroup>
</div>
);
Expand Down