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

PopradiArpad · 2017-06-06T16:51:10Z

Huhh... This should replace PR #12176
([UI Framework] Reactify kuiCard and related CSS components)
which got stuck at rebase :(

elasticmachine · 2017-06-06T16:51:12Z

Can one of the admins verify this patch?

weltenwort · 2017-06-06T17:20:38Z

ui_framework/components/card/card_group.js

+KuiCardGroup.propTypes = {
+  children: PropTypes.node,
+  className: PropTypes.string,
+  isUnited: PropTypes.bool


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

It sounds good.

weltenwort · 2017-06-06T17:38:50Z

ui_framework/doc_site/src/views/card/card_group.js

+} from '../../../../components';
+
+export default () => {
+  const cardStyle = {


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. */

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

weltenwort · 2017-06-06T17:43:08Z

And before I forget it: It's awesome that you invest the effort into this, @PopradiArpad. Thank you for contributing! ❤️

PopradiArpad · 2017-06-07T08:03:06Z

@weltenwort Thanks for your review!

PopradiArpad · 2017-06-07T08:21:32Z

Better example layout. See the comment of @cjcenizal at #12176

cjcenizal

Woohoo! Looks fantastic! Huge improvement to the code. I had one small comment about formatting and then we can merge. @weltenwort could you take another look?

cjcenizal · 2017-06-09T00:29:43Z

ui_framework/doc_site/src/views/card/card_group.js

+
+  return (
+    <div>
+        <KuiCardGroup>


I think we need to outdent this entire block by two spaces (up to line 89).

cjcenizal · 2017-06-09T00:30:48Z

jenkins, test this

cjcenizal · 2017-06-09T05:23:03Z

Looks like there is a minor conflict now because we merged one of your other PRs. Are you OK with rebasing master and resolving the merge conflict? I added instructions on how to do this to our CONTRIBUTING.md doc: https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#rebasing-and-fixing-merge-conflicts

…nents

…ed -> isUnited, use other framework components in doc

…styling

PopradiArpad · 2017-06-09T08:40:45Z

@cjcenizal Super doc! Thanks for the review!

weltenwort

I just left a small correction and something related to the general BEM compliance of the kui framework. This should probably not block this PR since it is a broader issue. 😉

weltenwort · 2017-06-09T10:53:11Z

ui_framework/components/card/_card_group.scss

 */
 .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 {


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:

https://en.bem.info/methodology/css/#openclosed-principle

https://cssguidelin.es/#the-openclosed-principle

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

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

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:

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.

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?

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.

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

for the record, the aforementioned PR is #12276

weltenwort · 2017-06-09T10:55:26Z

ui_framework/doc_site/src/views/card/card_group.js

@@ -12,58 +11,82 @@ import {
 } from '../../../../components';

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


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

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

cjcenizal · 2017-06-12T15:43:33Z

jenkins, test this

cjcenizal · 2017-06-12T22:31:10Z

jenkins, test this dangit!

cjcenizal · 2017-06-13T00:07:00Z

Woohoo!! 🎯

This was referenced Jun 6, 2017

[UI Framework] Reactify the kuiCard and related CSS components #12170

Closed

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

Closed

weltenwort requested changes Jun 6, 2017

View reviewed changes

cjcenizal approved these changes Jun 9, 2017

View reviewed changes

cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. release_note:enhancement v6.0.0 v6.0.0-beta1 labels Jun 9, 2017

PopradiArpad added 6 commits June 9, 2017 10:35

issue/12170 [UI Framework] Reactify the kuiCard and related CSS compo…

db71202

…nents

adapt doc views of card to simpler CSS syntax, fix property name unit…

84eecae

…ed -> isUnited, use other framework components in doc

add defaultProps to KuiCardGroup, add hint to example code

e5b2e1f

use 3 cards in the card group example to see the layout better

c36765d

remove KuiCardGroupContainer: not needed with the right KuiCardGroup …

23acc84

…styling

fix indentation in ui_framework/doc_site/src/views/card/card_group.js

d8373b6

PopradiArpad force-pushed the uiframework/feature/reactify-card3 branch from 977da71 to d8373b6 Compare June 9, 2017 08:36

weltenwort reviewed Jun 9, 2017

View reviewed changes

fix typo in comment

6c3e802

weltenwort mentioned this pull request Jun 12, 2017

[docs] Add css style guide section on open/closed principle #12276

Merged

weltenwort approved these changes Jun 12, 2017

View reviewed changes

cjcenizal merged commit 1e71d11 into elastic:master Jun 13, 2017

PopradiArpad deleted the uiframework/feature/reactify-card3 branch June 13, 2017 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PopradiArpad commented Jun 6, 2017

elasticmachine commented Jun 6, 2017

weltenwort Jun 6, 2017

PopradiArpad Jun 7, 2017

weltenwort Jun 6, 2017

PopradiArpad Jun 7, 2017

weltenwort commented Jun 6, 2017

PopradiArpad commented Jun 7, 2017

PopradiArpad commented Jun 7, 2017

cjcenizal left a comment

cjcenizal Jun 9, 2017

PopradiArpad Jun 9, 2017

cjcenizal commented Jun 9, 2017

cjcenizal commented Jun 9, 2017

PopradiArpad commented Jun 9, 2017

weltenwort left a comment

weltenwort Jun 9, 2017

PopradiArpad Jun 9, 2017 •

edited

Loading

cjcenizal Jun 9, 2017

PopradiArpad Jun 9, 2017 •

edited

Loading

weltenwort Jun 12, 2017

weltenwort Jun 12, 2017

weltenwort Jun 9, 2017

PopradiArpad Jun 9, 2017 •

edited

Loading

cjcenizal commented Jun 12, 2017

cjcenizal commented Jun 12, 2017

cjcenizal commented Jun 13, 2017

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

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

Conversation

PopradiArpad commented Jun 6, 2017

elasticmachine commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltenwort commented Jun 6, 2017

PopradiArpad commented Jun 7, 2017

PopradiArpad commented Jun 7, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jun 9, 2017

cjcenizal commented Jun 9, 2017

PopradiArpad commented Jun 9, 2017

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PopradiArpad Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PopradiArpad Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PopradiArpad Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Jun 12, 2017

cjcenizal commented Jun 12, 2017

cjcenizal commented Jun 13, 2017

PopradiArpad Jun 9, 2017 •

edited

Loading

PopradiArpad Jun 9, 2017 •

edited

Loading

PopradiArpad Jun 9, 2017 •

edited

Loading