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] Reactify kuiCard and related CSS components #12197

Merged
Merged
11 changes: 11 additions & 0 deletions ui_framework/components/card/__snapshots__/card.test.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders KuiCard 1`] = `
<div
aria-label="aria-label"
class="kuiCard testClass1 testClass2"
data-test-subj="test subject string"
>
children
</div>
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders KuiCardDescription 1`] = `
<div
aria-label="aria-label"
class="kuiCard__description testClass1 testClass2"
data-test-subj="test subject string"
>
children
</div>
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders KuiCardDescriptionText 1`] = `
<div
aria-label="aria-label"
class="kuiCard__descriptionText testClass1 testClass2"
data-test-subj="test subject string"
>
children
</div>
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders KuiCardDescriptionTitle 1`] = `
<div
aria-label="aria-label"
class="kuiCard__descriptionTitle testClass1 testClass2"
data-test-subj="test subject string"
>
children
</div>
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders KuiCardFooter 1`] = `
<div
aria-label="aria-label"
class="kuiCard__footer testClass1 testClass2"
data-test-subj="test subject string"
>
children
</div>
`;
21 changes: 21 additions & 0 deletions ui_framework/components/card/__snapshots__/card_group.test.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders KuiCardGroup 1`] = `
<div
aria-label="aria-label"
class="kuiCardGroup testClass1 testClass2"
data-test-subj="test subject string"
>
children
</div>
`;

exports[`renders KuiCardGroup isUnited 1`] = `
<div
aria-label="aria-label"
class="kuiCardGroup testClass1 testClass2 kuiCardGroup--united"
data-test-subj="test subject string"
>
children
</div>
`;
24 changes: 13 additions & 11 deletions ui_framework/components/card/_card_group.scss
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

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

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
13 changes: 13 additions & 0 deletions ui_framework/components/card/card.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';

export const KuiCard = ({ children, className, ...rest }) => {
const classes = classNames('kuiCard', className);
return <div className={classes} {...rest}>{children}</div>;
};
KuiCard.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
};
12 changes: 12 additions & 0 deletions ui_framework/components/card/card.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
KuiCard,
} from './card';

test('renders KuiCard', () => {
const component = <KuiCard { ...requiredProps }>children</KuiCard>;
expect(render(component)).toMatchSnapshot();
});
13 changes: 13 additions & 0 deletions ui_framework/components/card/card_description.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';

export const KuiCardDescription = ({ children, className, ...rest }) => {
const classes = classNames('kuiCard__description', className);
return <div className={classes} {...rest}>{children}</div>;
};
KuiCardDescription.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
};
12 changes: 12 additions & 0 deletions ui_framework/components/card/card_description.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
KuiCardDescription,
} from './card_description';

test('renders KuiCardDescription', () => {
const component = <KuiCardDescription { ...requiredProps }>children</KuiCardDescription>;
expect(render(component)).toMatchSnapshot();
});
13 changes: 13 additions & 0 deletions ui_framework/components/card/card_description_text.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';

export const KuiCardDescriptionText = ({ children, className, ...rest }) => {
const classes = classNames('kuiCard__descriptionText', className);
return <div className={classes} {...rest}>{children}</div>;
};
KuiCardDescriptionText.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
};
12 changes: 12 additions & 0 deletions ui_framework/components/card/card_description_text.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
KuiCardDescriptionText,
} from './card_description_text';

test('renders KuiCardDescriptionText', () => {
const component = <KuiCardDescriptionText { ...requiredProps }>children</KuiCardDescriptionText>;
expect(render(component)).toMatchSnapshot();
});
13 changes: 13 additions & 0 deletions ui_framework/components/card/card_description_title.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';

export const KuiCardDescriptionTitle = ({ children, className, ...rest }) => {
const classes = classNames('kuiCard__descriptionTitle', className);
return <div className={classes} {...rest}>{children}</div>;
};
KuiCardDescriptionTitle.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
};
12 changes: 12 additions & 0 deletions ui_framework/components/card/card_description_title.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
KuiCardDescriptionTitle,
} from './card_description_title';

test('renders KuiCardDescriptionTitle', () => {
const component = <KuiCardDescriptionTitle { ...requiredProps }>children</KuiCardDescriptionTitle>;
expect(render(component)).toMatchSnapshot();
});
13 changes: 13 additions & 0 deletions ui_framework/components/card/card_footer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';

export const KuiCardFooter = ({ children, className, ...rest }) => {
const classes = classNames('kuiCard__footer', className);
return <div className={classes} {...rest}>{children}</div>;
};
KuiCardFooter.propTypes = {
children: PropTypes.node,
className: PropTypes.string,
};
12 changes: 12 additions & 0 deletions ui_framework/components/card/card_footer.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
KuiCardFooter,
} from './card_footer';

test('renders KuiCardFooter', () => {
const component = <KuiCardFooter { ...requiredProps }>children</KuiCardFooter>;
expect(render(component)).toMatchSnapshot();
});
19 changes: 19 additions & 0 deletions ui_framework/components/card/card_group.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';

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,
isUnited: PropTypes.bool
Copy link
Member

Choose a reason for hiding this comment

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

If isUnited is not required, we should provide a value in defaultProps for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds good.

};
17 changes: 17 additions & 0 deletions ui_framework/components/card/card_group.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
KuiCardGroup,
} from './card_group';

test('renders KuiCardGroup', () => {
const component = <KuiCardGroup { ...requiredProps }>children</KuiCardGroup>;
expect(render(component)).toMatchSnapshot();
});

test('renders KuiCardGroup isUnited', () => {
const component = <KuiCardGroup isUnited { ...requiredProps }>children</KuiCardGroup>;
expect(render(component)).toMatchSnapshot();
});
6 changes: 6 additions & 0 deletions ui_framework/components/card/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export { KuiCard } from './card';
export { KuiCardDescriptionText } from './card_description_text';
export { KuiCardDescriptionTitle } from './card_description_title';
export { KuiCardDescription } from './card_description';
export { KuiCardFooter } from './card_footer';
export { KuiCardGroup } from './card_group';
9 changes: 9 additions & 0 deletions ui_framework/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@ export {
KuiSubmitButton,
} from './button';

export {
KuiCard,
KuiCardDescriptionText,
KuiCardDescriptionTitle,
KuiCardDescription,
KuiCardFooter,
KuiCardGroup,
} from './card';

export {
KuiCollapseButton
} from './collapse_button';
Expand Down
Loading