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

feat(Flex): improve gap and rowGap behaviour #4496

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

snorrekim
Copy link
Contributor

@snorrekim snorrekim commented Jan 24, 2025

Changes

  • give rowGap the same sizes as gap by adding x-small, xx-small, x-large and xx-large
  • stop allowing both gap and rowGap to add flex-gap/spacing at the same time in vertical layout. Only use the prop set by the user. If both are set, rowGap will be used.

Minor changes

  • remove value true from rowGap , it will treat it the same as undefined (results in 'small')
  • rowGap is no longer affected by the use of size or a heading in children items
  • Bugfix: The css variable --gap will no loner be erroneously inherited by parent <Flex> components

Copy link

vercel bot commented Jan 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eufemia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm

Copy link

codesandbox-ci bot commented Jan 24, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@snorrekim
Copy link
Contributor Author

Perhaps we should just set the ccs variable --gap: 0 as default to avoid having the .dnb-flex-container--row-gap-off class everywhere?

@snorrekim
Copy link
Contributor Author

snorrekim commented Feb 4, 2025

Some notes for future discussions:

Today:

  • gap uses margins
  • rowGap uses flex gap
  • rowGap is turned off in vertical mode by default, and gap is used instead
  • but you can still set rowGap and if you do, it is added to gap, it does not replace it.

Vertical mode is the place where the most confusion exists.

  • if i set rowGap to false there is still a gap because of the gap prop
  • Should gap and rowGap add together, or should one overwrite the other?
  • Do we have a reason for not wanting to use flex gap for vertical layouts?
  • Is it ok that rowGap has a different default value in vertical and horizontal?

@snorrekim
Copy link
Contributor Author

snorrekim commented Feb 4, 2025

What to do when props can overlap?

The issues only really affect vertical direction. So heres some examples for discussion:

Today: gap and rowGap is added, but rowGap is zero by default
PR: rowGap is ignored
REV: gap is ignored
SET: unset props are zero, unless no prop is set, then gap or rowGap is small

Always makes sense

<Flex.Vertical >
// Today: small (margin)
// PR: small (margin)
// REV: small (flex)
// SET: small (flex/margin)

Is always strange, this is the one we should decide on

<Flex.Vertical gap="large" rowGap="medium" >
// Today: large (margin) + medium (flex) 
// PR: large (margin) 
// REV: medium (flex)
// SET: large (margin) + medium (flex) 

"REV" and "SET" makes sense

<Flex.Vertical rowGap="medium" >
// Today: small (margin) + medium (flex)
// PR: small (margin)
// REV: medium (flex)
// SET: medium (flex)

"PR" and "SET" makes sense

<Flex.Vertical gap="large" >
// Today: large (margin)
// PR: large (margin)
// REV: small (flex)
// SET: large (margin)

@snorrekim
Copy link
Contributor Author

Margin or flex-gap?

Right now the gap and rowGap are trying to answer two different questions at the same time. "Should we use margin or flex-gap?" and "what should decide row gap?`

If we can answer one of those questions, then we could more easily fix the code.

  • Do we need to give the option of using both margin and flex-gap?
  • Do we want colGap as well perhaps?
  • Is margin only needed for horizontal gaps?

@snorrekim
Copy link
Contributor Author

Turns out we need to use margins like gap vertically as well in order to allow individual child items to overwrite their spacing. So rowGap. will still override gap, but we will be using the margin spacing of gap.

@snorrekim
Copy link
Contributor Author

Just for documentation, this is the original description before revisions:

Explanation

The current behaviour of gap and rowGap has too many pitfalls and nonsensical logic. For example, the rowGap had the following behaviour "in horizontal mode, if you set the size of a child item, rowGap defaults to "small", but if the user tries to set rowGap, then rowGap is set to the same as gap, but if gap was x/xx small/large, then it's set to 0" or "rowGap={true} only does something if you are in vertical mode" or vertical mode still has a gap even if you set rowGap={false}`

It's too many different cases and impossible to properly explain. The new "rules" are as simple as possible without loosing functionality:

  • size of children, or whether there is a heading or not in the children no longer affect rowGap.
  • rowGap no longer interferes with gap in vertical layouts

There's some more cleanup that might be needed on the logic around heading child and lines between stacks, but at least it shouldn't affect the gaps.

Other minor changes:

  • deprecated rowGap={true}, it is now the same as undefined (which defaults to the same value as true did: 'small')
  • ensure we set css variable --gap: 0 to prevent parent <Flex> from setting --gap.

Potential for issues:

Vertical layout has some changed behaviour:

  • rowGap={true} used to mean "small + gap on vertical layout", now it means "gap"
  • can no longer combine gap and ´rowGap` spacing

@snorrekim snorrekim marked this pull request as ready for review February 6, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant