-
Notifications
You must be signed in to change notification settings - Fork 676
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
feat(@theme-ui/components) Add a Stack component based on Flex layout #850
Conversation
Documentation is done, the component seems to work as expected and I've added tests. All yours @jxnblk! |
@janhoogeveen sweet! thanks for working on this! I'll try to give this a review over the next few days |
|
||
export const merge = (...objs) => { | ||
return objs.reduce(function(merged, currentValue) { | ||
return mergeTwo(merged, currentValue) |
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.
possibly jumping in w/o enough context, but where is mergeTwo
coming from in this case?
styles['> *:not(:last-child)'] = direction.map(d => createSpace(d, space)) | ||
} else { | ||
styles.flexDirection = direction === 'vertical' ? 'column' : 'row' | ||
styles['> *:not(:last-child)'] = createSpace(direction, space) |
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 think it would be better to use the lobotomized owl selector (> * + *
) instead here so it would not override any margin defined on the children.
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 disagree, I think adding any complex selectors (either this current implementation or the lobotomized owl) here is dangerous to be honest.
A lot of Theme UI is based on the Box
primitive and anything like this is going to lead to specificity issues
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.
Something simimlar to what Braid does could be done, they add a bottom spacing to each item (using padding), and add a negative margin using the same spacing to the container.
Reference:
- https://github.com/seek-oss/braid-design-system/blob/master/lib/components/Stack/Stack.tsx#L116
- https://github.com/seek-oss/braid-design-system/blob/master/lib/components/Stack/Stack.tsx#L31
And you can inspect an example on their playroom
With this approach no selectors would be needed!
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 the approach I'm using in raam
too and works rather nicely (although it needs a tidy up)
whoops, saw my repo in the description as inspiration please don't take the stack implementation from react-ui, it's doesn't follow the spec proposal (yet) further reading if you're curious: https://sid.st/unpolished/flex-gap-polyfill/ |
Although I really appreciate the work put into this, using Side note: If anyone's coming here looking for a |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/systemui/theme-ui/8i79v3pt3 |
This PR is an attempt at solving #556
Prior art
I've taken inspiration from https://github.com/siddharthkp/react-ui/blob/master/packages/react-ui/src/components/stack/index.js
Technical approach
This solution uses Flexbox to create stack layouts. I was afraid that by using css selectors you wouldn't be able to override margins anymore, but turns out you can quite easily work your way around it.
For example, you can use a negative value in your child component to negate the gap property:
Tradeoffs
Flex components are more complex than a simple grid component.
Alternative approach
This is an alternative, more powerful version of #809