-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][K7]: Flex Group and Flex Grid components #13603
Conversation
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.
This is badass! I love it. How about adding an example of nested FlexGroups? I also had a few other suggestions.
@@ -0,0 +1,60 @@ | |||
.kuiFlexGrid { |
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.
I know you hate calc
, but how about we try this?
.kuiFlexGrid {
display: flex;
align-items: stretch;
flex-wrap: wrap;
margin-bottom: 0;
.kuiFlexItem {
flex-grow: 0;
}
}
$gutterTypes: (
gutterSmall: $kuiSizeS / 2,
gutterMedium: $kuiSize / 2,
gutterLarge: $kuiSizeL / 2,
gutterExtraLarge: $kuiSizeXL / 2,
);
$fractions: (
flexBasisFourths: 25%,
flexBasisThirds: 33.3%,
flexBasisHalves: 50%,
);
@each $gutterName, $gutterSize in $gutterTypes {
@each $fraction, $percentage in $fractions {
$halfGutterSize: $gutterSize * 0.5;
.kuiFlexGrid--#{$gutterName} {
margin: -$halfGutterSize;
&.kuiFlexGrid--#{$fraction} {
.kuiFlexItem {
margin: $halfGutterSize;
flex-basis: calc(#{$percentage} - #{$gutterSize});
}
}
}
}
}
This will apply my trick from http://www.flexboxpatterns.com/gallery and get rid of the extra margin on the top row of FlexItems.
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.
This was applied. Your gutter sizes vars were unnecessarily halved, so I changed that one bit. (They still apply margins by halves).
} | ||
|
||
// Grid columns | ||
&.kuiFlexGrid--flexBasisFourths .kuiFlexItem { |
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.
Might be a small improvement to rename this --fourths
, --thirds
, etc to simplify the modifier name and avoid leaking implementation details into the interface.
margin-left: $kuiSizeXXL; | ||
} | ||
|
||
// Turn off item grown |
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.
Is "grown" a typo?
small: 'kuiFlexGrid--gutterSmall', | ||
medium: 'kuiFlexGrid--gutterMedium', | ||
large: 'kuiFlexGrid--gutterLarge', | ||
extraLarge: 'kuiFlexGrid--gutterExtraLarge', |
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.
I'd be OK with xLarge
here if you are. If we have to go up to XXL, typing "extraExtraLarge" could be kinda silly.
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.
So I think I was following your own patterns here. You seemed to prefer longform in the selector names themselves.
gutterSize: PropTypes.oneOf(GUTTER_SIZES), | ||
alignItems: PropTypes.oneOf(ALIGN_ITEMS), | ||
justifyContent: PropTypes.oneOf(JUSTIFY_CONTENTS), | ||
growItems: PropTypes.bool, |
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.
What do you think of renaming alignItems
and justifyContent
to verticalAlign
and horizontalAlign
and changing the values they accept to top
, bottom
, center
, stretch
and left
, right
, center
, spaceAround
, and spaceBetween
, respectively, and also renaming the corresponding classes? I think people will find this much more intuitive.
I'd also like to find a name for growItems
that begins with "is", like isStretchToFit
, since it's a boolean.
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.
I'd like to keep it as close to the flex box naming as possible, so it telegraphs the prop values you can pass.
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.
OK, we can go with this for now and change it later if people find it awkward. You might also want to ping @chrisronline for his opinion on which is easier to grok.
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.
I personally think keeping it as close to the CSS property names will be easier in the long term
} from '../../../../components'; | ||
|
||
export default () => ( | ||
<div> |
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 examples with a single root element, like this one, we can get rid of the wrapper <div>
to clean things up a bit.
} from '../../../../components'; | ||
|
||
export default () => ( | ||
<div> |
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.
Same here, and in the other examples.
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.
This one wasn't a single root.
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.
Oops :)
<br/><br/> | ||
|
||
<KuiFlexGroup growItems={false} justifyContent="spaceAround"> | ||
<KuiFlexItem>I'm a single centered item!</KuiFlexItem> |
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.
Could you run npm i
? I think you might have an older linter installed. The linter should complain about this apostrophe and request you use an HTML entity instead (’
).
FlexGroup and FlexGrid allows for layout and positioning inside content.
FlexGroup and FlexGrid allows for layout and positioning inside content.
Adds components to manage basic column structures within content. On mobile layouts will act responsively and collapse.
cc @chrisronline. Once in, this will give you the ability to split pages into columns.
TODO in later PR
Preview at http://snid.es/1u0I0C0g110A