feat(spacings): support justify-content prop for Inline/Stack components

[emmenko]

Fixes #552

Additional change

I also deprecated the alignItems values written in camelCase, which I found weird as we should simply pass the normal css values. So the "old" values are still supported for backwards compatibility.

[qmateub]

Code looks good to me! Looking forward to have this in 🎉

Should we add also the justifyContent to the story of Stack and Inline? 🤔

[emmenko]

Should we add also the justifyContent to the story of Stack and Inline? 🤔

I won't affect the story, because of how it's being setup. We could add more examples in the story but then we're just testing how flexbox works, so I opted to just keep it as is.

[montezume]

Can you also add a VRT for this?

[emmenko]

What do you suggest to test in the VRT?

[emmenko]

Or do you mean VRT for spacing components in general?

[montezume]

Or do you mean VRT for spacing components in general?

@emmenko just make a test using this prop? Add it to the spacings vrt test in the stack and inline sections? 🙏

_{Sent with GitHawk}

[tdeekens]

Should we warning when they're used?

[emmenko]

Hmm that might end up with a lot of noise. We can maybe do that in the next major, then in the major after that we can remove them completely.
So a two-step deprecation process.

[emmenko]

@emmenko just make a test using this prop? Add it to the spacings vrt test in the stack and inline sections? 🙏

Sure, I'll see what I can do

[emmenko]

Alright, I added some VRT for the justifyContent prop. I also removed the prop from the Stack component, as I realised that it's pretty much useless for flex columns (at least I couldn't find a use case for that).

[montezume]

This is the VRT for inset not inline though 🤔

[emmenko]

Sorry just a typo

[ahmehri]

@emmenko I was going to request supporting justify-content for the component Spacings.Stack. I first searched the repository which led me to this PR. The title insinuates that it's already supported, but when I checked the changes it's not. Any reason why the support wasn't added to the component Spacings.Stack as well?

[emmenko]

@ahmehri in general, for vertical items (spacing stack) justify-content only works if the container has a fixed height. This is usually not the case for a spacing stack component, as the height is not defined and determined by the number of items and their dimension.

Can you describe your use case?

[ahmehri]

Yes, my use case is positioning two items vertically with a container that has a fixed height, and position them in a way that there is a space between them.

I achieved that using the component Grid, but Flexbox layout (the Spacings.Stack component) is more suitable here because of having only one dimension. The same can be achieved using Spacings.Stack if justify-content was supported (of course the container still needs to have a fixed height).

<Spacings.Stack style={{ height: "100%" }} alignContent="space-between">
  <ImageLabel />
  <Controls />
</Spacings.Stack>

[emmenko]

I see. Given that this is a special case anyway (fixed height, etc), I would suggest maybe to use a <div> with custom styles, as you only want a flex container and the spacing is not an important factor.

Does that make sense?

[ahmehri]

In a way, but the idea is to use the ui-kit component in favor of the custom styles (the ultimate goal is to get rid of all the custom styles). Given that the component Spacings.Stack works in both cases (whether the container has a fixed height), I don't see why we should not support the property justify-content. The idea of using the component is to be able to use the Flexbox layout using a React component instead of writing a custom CSS. Not supporting the property justify-content is a limitation in my opinion.

Another argument is to achieve symmetry between components Spacings.Inline (flex-direction: row) and Spacings.Stack (flex-direction: column). The former supports both properties justify-content and align-items whereas the latter supports only one.

[emmenko]

One thing to keep in mind is that the spacing components are not the same as flex box containers. Yes, they use flex but they aim to solve a specific use case.

I don't have a problem with adding the justifyContent prop, that's not the point.

Even if we add that, you still need to solve the problem with the fixed height to solve your use case, right?
So does that mean we need to also add a height prop?

Or how did you plan to solve that issue?

PS: we can also think if it makes sense to add a <Flex> component, similarly to <Grid>.

[ahmehri]

You're right, passing prop style (style={{ height: "100%" }} to set the fixed height) to the component Spacings.Stack won't have any effect. This is not the case for the component Grid, in fact, that's how I was able to set the fixed height for the grid container.

One thing to keep in mind is that the spacing components are not the same as flex box containers. Yes, they use flex but they aim to solve a specific use case.

I'm more convinced now about this and think adding a new component Flex as Grid is the best solution.

[emmenko]

Sounds good then, PR welcomed. PS: it can also be an onboarding task if you want 😉

First thing maybe to open an issue describing the use case. Thanks 🙏