Conversation
kuzhelov
requested review from
dvdzkwsk,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
kuzhelov
changed the title
Codecov Report
@@ Coverage Diff @@
## master #802 +/- ##
=======================================
Coverage 93.54% 93.54%
=======================================
Files 21 21
Lines 728 728
Branches 69 73 +4
=======================================
Hits 681 681
Misses 47 47Continue to review full report at Codecov.
|
… into feat/flex-layout
bmdalex
left a comment
bmdalex
left a comment
There was a problem hiding this comment.
very nice job, pls take a look at my comments
| const renderGap = index !== 0 | ||
| return ( | ||
| <> | ||
| {renderGap && gap && <Flex.Gap className={gapClasses} />} |
There was a problem hiding this comment.
the gap logic can be done only once before the loop;
private renderChild: (
childElement: React.ReactChild,
renderGap: boolean,
gapClasses: any // type for classes
) => React.ReactChild = this.props.gap
? (childElement, renderGap, gapClasses) => (
<>
{renderGap && <Flex.Gap className={gapClasses} />}
{childElement}
</>
)
: childElement => childElementand use it as:
return this.renderChild(childElement, index !== 0, gapClasses)in your React.Children.map function?
Looking at it again it reduces readability a bit so feel free to disregard..
| 'size.half': '50%', | ||
| 'size.quarter': '25%', | ||
|
|
||
| 'size.small': pxToRem(150), |
There was a problem hiding this comment.
how did we come up with these values?
can we make these part of theme somehow or is that plan for future?
There was a problem hiding this comment.
it is definitely a plan for the future - there are no clear specs on that yet, provided numbers are free to be changed
| </Flex.Item> | ||
| </Flex> | ||
|
|
||
| <Flex gap="gap.small" padding="padding.medium" style={{ minHeight: 200 }}> |
There was a problem hiding this comment.
what about having height as prop? I imagine it was considered already? Disregard in that case
There was a problem hiding this comment.
yes, actually, it was - and the main reason for not doing it (for now, at least) is that height wasn't seen as a close attribute of the layout aspects - in other case, this prop is not specific to layout component (the same way then we might argue about adding height to API of Segment).
Also, note that if Flex needs to be provided with some height while being part of some other component, this will be done by styles, not by the props API:
renderComponent = ({ ..., ElementType, styles }) => {
<ElementType>
<Flex styles={styles.flex} >
....
</Flex>
</ElementType>
}
/// myComponentStyles.js
{
root: ...
flex: {
height: '200px'
}
}
docs/src/examples/components/Flex/Types/FlexExampleColumns.shorthand.tsx
Show resolved
Hide resolved
docs/src/examples/components/Flex/Types/FlexExampleItemsAlignment.shorthand.tsx
Outdated
Show resolved
Hide resolved
| import FlexItem from './FlexItem' | ||
|
|
||
| export type FlexGap = 'gap.small' | 'gap.medium' | 'gap.large' | ||
| export type FlexPadding = 'padding.medium' |
There was a problem hiding this comment.
Why we have just one option here. Moreover why is the value medium? It doesn't make sense without any other values in my opinion.
There was a problem hiding this comment.
agree - but lets refine the set of padding values further down the road. What I am not sure at the moment is whether padding should be specified this way, as it is a 2D thing - what if, for instance, we would need to make vertical padding lesser than horizontal one, as it is usually the case for button? medium name was selected just to be consistent with the dictionary we've used so far for size values.
With that being said, I acknowledge the issue, but it was intentionally left visible, as the approach to organise these values (by putting them under general design schema) needs to be introduced as a follow-up work.
Codecov Report
@@ Coverage Diff @@
## master #802 +/- ##
=======================================
Coverage 93.54% 93.54%
=======================================
Files 21 21
Lines 728 728
Branches 69 73 +4
=======================================
Hits 681 681
Misses 47 47Continue to review full report at Codecov.
|
5 participants
Principles
gap,size,paddingprops) that should follow foreseen approach to design tokens' namesLayout example sources used
Important cases to cover
API and Vocabulary
FlexContainer element.
column: booleanhAlign: 'start' | 'center' | 'end' | 'stretch',vAlign: ...center,top, ...) - as those will make it impossible to override related effects:<Flex {...} center /> - there is no guarantee that center alignment will be appliedspace: 'between' | 'around' | 'evenly' | ...gap: 'gap.small' | 'gap.medium' | 'gap.large'inline: booleanwrap: boolean- allows items to wrap on the next linepadding: 'padding.medium'Flex.ItemShould be sparingly used, only in cases where styles for individual item should be introduced.
size: 'size.small' | 'size.medium' | 'size.half' | 'size.quarter'grow: boolean | ValueOf<FlexGrow>shrink: boolean | ValueOf<FlexShrink>align: 'start | center | end | ... 'push: booleanQ&A
FlexorRow(or other name)?Flexand get on par with CSS Flex capabilities (while intentionally limiting some features, such as reordering of child DOM elements)gapandpaddingofFlexsizeofFlex.Item