-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
cjcenizal
merged 7 commits into
elastic:master
from
PopradiArpad:uiframework/feature/reactify-card3
Jun 13, 2017
Merged
Changes from 6 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 84eecae
adapt doc views of card to simpler CSS syntax, fix property name unit…
PopradiArpad e5b2e1f
add defaultProps to KuiCardGroup, add hint to example code
PopradiArpad c36765d
use 3 cards in the card group example to see the layout better
PopradiArpad 23acc84
remove KuiCardGroupContainer: not needed with the right KuiCardGroup …
PopradiArpad d8373b6
fix indentation in ui_framework/doc_site/src/views/card/card_group.js
PopradiArpad 6c3e802
fix typo in comment
PopradiArpad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
11 changes: 11 additions & 0 deletions
11
ui_framework/components/card/__snapshots__/card.test.js.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> | ||
`; |
11 changes: 11 additions & 0 deletions
11
ui_framework/components/card/__snapshots__/card_description.test.js.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> | ||
`; |
11 changes: 11 additions & 0 deletions
11
ui_framework/components/card/__snapshots__/card_description_text.test.js.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> | ||
`; |
11 changes: 11 additions & 0 deletions
11
ui_framework/components/card/__snapshots__/card_description_title.test.js.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> | ||
`; |
11 changes: 11 additions & 0 deletions
11
ui_framework/components/card/__snapshots__/card_footer.test.js.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
21
ui_framework/components/card/__snapshots__/card_group.test.js.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> | ||
`; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
12
ui_framework/components/card/card_description_text.test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
12
ui_framework/components/card/card_description_title.test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It sounds good. |
||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
.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.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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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