Skip to content
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

Spacings component support option for space-between layout configuration #552

Closed
qmateub opened this issue Feb 18, 2019 · 5 comments
Closed
Labels
💅 Type: Enhancement Improves existing code

Comments

@qmateub
Copy link
Contributor

qmateub commented Feb 18, 2019

Problem

Currently we have a bunch of files in the MC that use custom style for the layout configuration of space-between. I'm thinking that if that is a common case for styling UI maybe we can add it to UI-Kit 😋

{
  display: flex;
  justify-content: space-between;
}

Proposal

The solution would be to just extends our Spacings component to support this option or maybe just add a new section called Layouts even though I think it makes more sense to add it to Spacings.

@emmenko already introduced some code in which we can get some inspiration from 👍
https://github.com/commercetools/merchant-center-application-kit/blob/011e07d2855c40b0c9d7b40ff463f467f6336362/packages/application-components/src/components/dialogs/info-dialog/info-dialog.js#L14-L20

The idea could be Spacings.Between or Spacings.Stretched with a configuration for the alignItems that by default would be centered. That is up to discussion 🐤

I see only benefits from this:

  • 🍰 We add more layout configs to our Spacings.
  • 🍰 We can potentially remove a lot of custom styles or even css files in the MC.

Additional context

Asked by Malcolm in commercetools/merchant-center-application-kit#367 (comment) 🤓

@qmateub qmateub added the 💅 Type: Enhancement Improves existing code label Feb 18, 2019
@emmenko
Copy link
Member

emmenko commented Feb 18, 2019

I think we have two options:

  • allow to pass justifyContent to the Spacings.Inline component
  • have a new spacing component

I think it should be enough to go with the first approach.

@qmateub
Copy link
Contributor Author

qmateub commented Feb 18, 2019

  • allow to pass justifyContent to the Spacings.Inline component

Does it mean that if we pass justifyContent then we don't accept the scale? because I think it does not make much sense to support that combination right? (That's why I thought in a separate component), event though I get your point of getting this inside the context of the Inline because at the end the items are just in line 😅 👍

WDYT? 🤔

@emmenko
Copy link
Member

emmenko commented Feb 18, 2019

The scale should not be a problem, it's just the margin between the elements. I guess we can just try it in storybook and see how it goes

@tdeekens
Copy link
Contributor

@emmenko
Copy link
Member

emmenko commented Feb 18, 2019

I think that issue should be solved with a css-grid

emmenko added a commit that referenced this issue Feb 19, 2019
…nts (#557)

* feat(spacings): support justify-content prop for Inline/Stack components. Fixes #552

* chore: add vrt for inline spacing with justify-content

* refactor: remove justifyContent from stack component (not useful)

* chore: typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💅 Type: Enhancement Improves existing code
Projects
None yet
Development

No branches or pull requests

3 participants