Add numbers between 1 and 10 for flexGrow #144

zinckiwi · 2017-11-14T09:09:12Z

Here's the proposal for arbitrary (well, up to a certain point) weighting for FlexItem.

zinckiwi · 2017-11-14T09:18:18Z

Another immediate use case; I'd want about a 1/4 - 3/4 split here between version and type.

zinckiwi · 2017-11-14T09:31:03Z

Oh, and entry in the docs:

pugnascotia · 2017-11-14T11:18:57Z

src/components/flex/flex_item.js

@@ -7,6 +7,7 @@ export const EuiFlexItem = ({ children, className, grow, ...rest }) => {
    'euiFlexItem',
    {
      'euiFlexItem--flexGrowZero': !grow,
+      [`euiFlexItem--flexGrow${grow}`]: Number(parseFloat(grow)) === grow && grow >= 1 && grow <= 10


How come thing is parseFloat, as opposed to parseInt?

absentmindedness, that's why

pugnascotia · 2017-11-14T11:20:58Z

src/components/flex/flex_item.js

@@ -7,6 +7,7 @@ export const EuiFlexItem = ({ children, className, grow, ...rest }) => {
    'euiFlexItem',
    {
      'euiFlexItem--flexGrowZero': !grow,


Does this need guarding against a numeric value?

Don't think so. Any falsy value would work, proptypes says bool or number, so that's 0 or false.

pugnascotia · 2017-11-14T11:24:27Z

Just to check - do these layouts still collapse as you'd expect on mobile?

zinckiwi · 2017-11-14T11:35:56Z

Yep, collapsing works fine.

pugnascotia

LGTM

cjcenizal

This is sweet! I just had a couple minor comments.

cjcenizal · 2017-11-14T15:16:49Z

src/components/flex/flex_item.js

@@ -7,6 +7,7 @@ export const EuiFlexItem = ({ children, className, grow, ...rest }) => {
    'euiFlexItem',
    {
      'euiFlexItem--flexGrowZero': !grow,
+      [`euiFlexItem--flexGrow${grow}`]: Number(parseInt(grow)) === grow && grow >= 1 && grow <= 10


Can we add a prop type checker to contain these conditions, and throw errors if it fails?

Can this just be

[`euiFlexItem--flexGrow${grow}`]: grow

now?

Depends how much you trust people to fix prop type warnings I suppose! For the sake of that extra bit, I'd personally favour not adding superfluous classes, but can change if anyone contends otherwise (including yourself).

It can still be a boolean though, no?

(though even then we'd need to catch true)

edit: nm, 1 == true

edit edit: I must be tired. Precisely because true >= 1 === true we'd need to catch it

Hmm, my instinct is to simplify this as much as possible, which would mean relying on the prop-types error and the CSS to apply the correct styling. That way if someone passes in "11", debugging means checking 2 places instead of 3. And likewise adding support for 11 means making changes in 2 places instead of 3.

That being said, I don't feel very strongly about this. It's your call. 😄

EDIT: In terms of supporting the boolean value, then I think this will do the trick?

[`euiFlexItem--flexGrow${grow}`]: grow && grow > 0

cjcenizal · 2017-11-14T15:19:46Z

src-docs/src/views/flex/flex_grow_numeric.js

+export default () => (
+  <div>
+    <EuiFlexGroup>
+      <EuiFlexItem grow={1}>1</EuiFlexItem>


Can we add some additional content to this first item to demonstrate that it wraps without pushing the column wider?

cjcenizal

LGTM! Had one optional comment.

cjcenizal · 2017-11-14T16:16:58Z

src-docs/src/views/flex/flex_grow_numeric.js

-      <EuiFlexItem grow={2}>2</EuiFlexItem>
-      <EuiFlexItem grow={3}>3</EuiFlexItem>
+      <EuiFlexItem grow={2}>2<br />wraps content if necessary</EuiFlexItem>
+      <EuiFlexItem grow={3}>3<br />expands_to_fit_if_content_cannot_wrap</EuiFlexItem>


cjcenizal · 2017-11-14T16:17:52Z

src/components/flex/flex_item.js

+
+  if (validValues.indexOf(value) === -1) {
+    return new Error(
+      'Prop `' + propName + '` supplied to' + ' `' + componentName + '` must be a boolean or an integer between 1 and 10.'


If you want, you could use a template literal here:

`Prop \`${propName}\` supplied to \`'${componentName}\` must be a boolean or an integer between 1 and 10.`

zinckiwi · 2017-11-14T16:50:06Z

Rather than dancing around JS hoops with true vs 1, whitelisted the numeric values. That work?

snide

I think the implementation is fine. My only worry is that this flexibility might create some choose your own adventure grid systems.

Cons:

It requires stacking if you want two rows.
Because of that, it'll require you to use an EuiSpacer to manage the spacing between those rows. You'd need to know to match the spacing size against the gutter size if you used anything but the defaults.

Pros:

As long as you're aware of those cons, you can configure any layout you likely want. But you'll need to hand construct it.

Possible alternative

We also apply this to EuiFlexGrid and allow the grow prop to take on the number of columns passed to the parent wrapper. Meaning I could set columns={12} and pass grow={4}, grow={4}, grow={4}, grow={6}, grow={6} on flex items inside to create 3 columns on row 1 and 2 columns on row 2. This would allow for more conventional wraps and act similar to similar grid systems. The downside here is that "grow" would mean two different things depending upon the wrapper (grow in Group is proportional against a total, grow in Grid is proportional against a passed number of allowed columns).

I'm just spitballing here. If people are OK with Scott's implementation I think it's flexible enough to work how we'd want, but it would be off-pattern for how people normal consume grids (ands that OK, I'm just bringing it up).

FWIW we could add the grid style one in a later PR.

snide · 2017-11-14T16:34:37Z

src-docs/src/views/flex/flex_grow_numeric.js

+      <EuiFlexItem grow={4}>4</EuiFlexItem>
+    </EuiFlexGroup>
+
+    <br/><br/>


This should be an EuiSpacer. It will create a vertical margin to match the horizontal ones.

(I copied from an adjacent block, FYI)

Then change it there too if you have time. Likely I built this stuff before I had spacer built. 😢

I always assume people will copy code out of the docs literally, so we should show them how to properly space stuff when we can.

Updated the instances of <br /><br /> in the flex area.

cjcenizal · 2017-11-14T18:03:10Z

Rather than dancing around JS hoops with true vs 1, whitelisted the numeric values. That work?

👍

@snide If the additions you have in mind won't require changing or removing the functionality added in this PR, then I think this is a good step forward. The cons you mention sound like good things to be aware of when building layouts with these tools.

@zinckiwi Maybe in a subsequent PR you can add a sandbox which demonstrates a layout using EuiFlexGroup, EuiFlexItem and EuiSpacer, so we can have a copy/pasteable example of best practices?

snide · 2017-11-14T18:28:25Z

@cjcenizal Yeah, I agree. This is fine to merge. My stuff can go in later, and more likely, we'll need to create better "layout" documentation.

cjcenizal · 2017-11-15T00:08:01Z

It's great that we're adding this because I actually need this now in Kibana!

Desired layout:

What I can do right now:

* Add numbers between 1 and 10 for flexGrow * Parse grow prop better * Add more granular prop type checking to FlexItem and expand doc * Add changelog entry * Switch string to interpolated * Be explicit with grow prop className building * Clean up double-brs in docs

Add numbers between 1 and 10 for flexGrow

3bb4f19

zinckiwi requested review from gjones, snide, bevacqua, cjcenizal and pugnascotia November 14, 2017 09:09

pugnascotia reviewed Nov 14, 2017

View reviewed changes

Parse grow prop better

b5b97a5

pugnascotia approved these changes Nov 14, 2017

View reviewed changes

cjcenizal suggested changes Nov 14, 2017

View reviewed changes

zinckiwi added 2 commits November 14, 2017 16:13

Add more granular prop type checking to FlexItem and expand doc

5803fe5

Add changelog entry

99221b5

cjcenizal approved these changes Nov 14, 2017

View reviewed changes

zinckiwi added 2 commits November 14, 2017 16:20

Switch string to interpolated

4f590c7

Be explicit with grow prop className building

cbbcbda

snide approved these changes Nov 14, 2017

View reviewed changes

Clean up double-brs in docs

f8e4f7a

zinckiwi merged commit 4105fd6 into elastic:master Nov 15, 2017

cjcenizal mentioned this pull request Nov 16, 2017

[UI Framework] Add support for wrap prop to FlexGroup elastic/kibana#15009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add numbers between 1 and 10 for flexGrow #144

Add numbers between 1 and 10 for flexGrow #144

zinckiwi commented Nov 14, 2017

zinckiwi commented Nov 14, 2017

zinckiwi commented Nov 14, 2017

pugnascotia Nov 14, 2017

zinckiwi Nov 14, 2017

pugnascotia Nov 14, 2017

zinckiwi Nov 14, 2017

pugnascotia commented Nov 14, 2017

zinckiwi commented Nov 14, 2017

pugnascotia left a comment

cjcenizal left a comment

cjcenizal Nov 14, 2017

cjcenizal Nov 14, 2017 •

edited

Loading

zinckiwi Nov 14, 2017

pugnascotia Nov 14, 2017

zinckiwi Nov 14, 2017 •

edited

Loading

cjcenizal Nov 14, 2017 •

edited

Loading

cjcenizal Nov 14, 2017

cjcenizal left a comment

cjcenizal Nov 14, 2017

cjcenizal Nov 14, 2017

zinckiwi commented Nov 14, 2017

snide left a comment

snide Nov 14, 2017

zinckiwi Nov 14, 2017

snide Nov 14, 2017

zinckiwi Nov 14, 2017

cjcenizal commented Nov 14, 2017

snide commented Nov 14, 2017

cjcenizal commented Nov 15, 2017

Add numbers between 1 and 10 for flexGrow #144

Add numbers between 1 and 10 for flexGrow #144

Conversation

zinckiwi commented Nov 14, 2017

zinckiwi commented Nov 14, 2017

zinckiwi commented Nov 14, 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

pugnascotia commented Nov 14, 2017

zinckiwi commented Nov 14, 2017

pugnascotia left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinckiwi Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

cjcenizal Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

zinckiwi commented Nov 14, 2017

snide left a comment

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

Choose a reason for hiding this comment

cjcenizal commented Nov 14, 2017

snide commented Nov 14, 2017

cjcenizal commented Nov 15, 2017

cjcenizal Nov 14, 2017 •

edited

Loading

zinckiwi Nov 14, 2017 •

edited

Loading

cjcenizal Nov 14, 2017 •

edited

Loading