Skip to content

add "container" prop to Cartesian#197

Merged
johno merged 4 commits intoc8r:masterfrom
VinSpee:master
Jun 23, 2018
Merged

add "container" prop to Cartesian#197
johno merged 4 commits intoc8r:masterfrom
VinSpee:master

Conversation

@VinSpee
Copy link
Copy Markdown
Contributor

@VinSpee VinSpee commented Jun 23, 2018

Allow the consumer to provide an optional container prop. This prop is the same type as the component prop and behaves the same way. The container prop is a way for consumers to control the way each child of the Cartesian component may look. I anticipate this being used primarily for spacing and layout, when the product of the cartesian component does not have any opinions about its container.

I ran into this need here:

<Cartesian
  component={Button}
  type={['normal', 'medium', 'dark']}
>
  {['Short', 'Really Really Long Label', 'Medium Label']}
</Cartesian>

image

I desired this output, and in order to get it I have to be allowed inside of Cartesian's .map fn. The only way I can see is the pass a prop:

const Wrap = withTheme(
  emotion.View(({ theme: t }) => ({
    padding: t.size(1),
  })),
);



<Cartesian
  container={Wrap}
  component={Button}
  type={['normal', 'medium', 'dark']}
>
  {['Short', 'Really Really Long Label', 'Medium Label']}
</Cartesian>

image

Any interest in adding something like this? as far as I can tell there's no way to do it from user-land as it currently stands.

VinSpee added 2 commits June 22, 2018 23:14
Allow the consumer to provide an optional `container` prop. This prop  is the same type as the `component` prop and behaves the same way. The `container` prop is a way for consumers to control the way each child of the Cartesian component may look. I anticipate this being used primarily for spacing and layout, when the product of the cartesian component does not have any opinions about its container.
since the container is now the outermost child, it needs the key.
Copy link
Copy Markdown
Member

@johno johno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Would definitely like to support this.

I left one comment for a potential small change. Aside from that lgtm ✨

Comment thread core/src/Cartesian.js Outdated
const combinations = cartesianProduct(props)
const Component = component

if (container) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we default container to Fragment so we only have one path for rendering?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great thinking. updated.

by defaulting the container prop to `Fragment`, we eliminate a set of possibilities and a [branch in the code](c8r#197 (comment)).
@VinSpee
Copy link
Copy Markdown
Contributor Author

VinSpee commented Jun 23, 2018

additionally, I'll add some coverage for this case.

@VinSpee
Copy link
Copy Markdown
Contributor Author

VinSpee commented Jun 23, 2018

wew lad

@johno
Copy link
Copy Markdown
Member

johno commented Jun 23, 2018

Thanks!

@johno johno merged commit 583724f into c8r:master Jun 23, 2018
@VinSpee
Copy link
Copy Markdown
Contributor Author

VinSpee commented Jun 23, 2018

released in v1.0.46!

any thoughts on docs? I'm happy to add it if you'd like

@johno
Copy link
Copy Markdown
Member

johno commented Jun 23, 2018

If you wanted to add a props table with descriptions/types that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants