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/themeable spacing #1180

Closed
wants to merge 4 commits into
base: development
from

Conversation

Projects
None yet
4 participants
@halfghaninne
Collaborator

halfghaninne commented Sep 7, 2018

@halfghaninne

This comment has been minimized.

Show comment
Hide comment
@halfghaninne

halfghaninne Sep 7, 2018

Collaborator

@nfiniteset My question around the display prop:

When the display prop is set to inline-block extra space appears between the Spacer component and it's surrounding components. This disappears if the group of components is wrapped in a parent whose display is flex.

I'm still curious what purpose the display property serves for the Spacer and if it is needed, assuming that Spacer will never have children or inner content.

Collaborator

halfghaninne commented Sep 7, 2018

@nfiniteset My question around the display prop:

When the display prop is set to inline-block extra space appears between the Spacer component and it's surrounding components. This disappears if the group of components is wrapped in a parent whose display is flex.

I'm still curious what purpose the display property serves for the Spacer and if it is needed, assuming that Spacer will never have children or inner content.

@nfiniteset

This comment has been minimized.

Show comment
Hide comment
@nfiniteset

nfiniteset Sep 7, 2018

Collaborator

I created a sandbox to explore this: https://codesandbox.io/s/xm371o74z

The sandbox lets you switch the display property of a spacer and its parent container. You can try different combinations.

The reason for exposing a display prop is about how the spacer behaves relative to its parent. It's not about the spacer having content.

Looks like for the spacer, having display: block or display: flex result in the same behavior no matter the parent's display property. When the spacer has display: inline-block though, it behaves differently.

Notice the spacer in the middle of the h2 and the one between the two select elements. Those both fall within an inline layout context. Having the spacer set to display: inline-block does what I expect there.

So the question is: will client devs use spacers in inline layout contexts? Hard to say. I expect them to be used that way less often than within flex or block layout contexts.

Ok. Making the example and writing all that helped me decide. Let's leave out the display prop until a client team asks for it. Thank you for pushing back on that!

Collaborator

nfiniteset commented Sep 7, 2018

I created a sandbox to explore this: https://codesandbox.io/s/xm371o74z

The sandbox lets you switch the display property of a spacer and its parent container. You can try different combinations.

The reason for exposing a display prop is about how the spacer behaves relative to its parent. It's not about the spacer having content.

Looks like for the spacer, having display: block or display: flex result in the same behavior no matter the parent's display property. When the spacer has display: inline-block though, it behaves differently.

Notice the spacer in the middle of the h2 and the one between the two select elements. Those both fall within an inline layout context. Having the spacer set to display: inline-block does what I expect there.

So the question is: will client devs use spacers in inline layout contexts? Hard to say. I expect them to be used that way less often than within flex or block layout contexts.

Ok. Making the example and writing all that helped me decide. Let's leave out the display prop until a client team asks for it. Thank you for pushing back on that!

@nfiniteset

Lots of good work here! Lots of new ideas too so I've made a bunch of comments.

Show outdated Hide outdated packages/spacer/src/__stories__/getKnobs.js
Show outdated Hide outdated packages/spacer/src/stylesheet.js
Show resolved Hide resolved packages/spacer/src/index.test.js
Show outdated Hide outdated packages/spacer/src/__gemini__/Spacer.stories-test.js
Show outdated Hide outdated packages/spacer/src/Spacer.test.js
Show outdated Hide outdated packages/spacer/src/Spacer.test.js
Show outdated Hide outdated packages/spacer/src/Spacer.js
Show resolved Hide resolved packages/spacer/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment