feat: Support CSS gap in Box, use it for spacing in Stack, Inline, Columns

[gnapse]

Short description

Introduce support for the CSS gap property.

References

• Closes [Box] Add gap as a property of the Box component #691
• gap on MDN
• Browser support on flexbox containers
• (Doist Internal) Recent Twist discussion deciding to support it

Test plan

Due to this being so far-reaching, let's do a bit more manual QA review here than usual. I personally worked over the entire test plan myself already¹, but a second pair of eyes would not hurt.

Test the library on its own

Run storybook locally npm run storybook while on this branch, and check the following:

Note
At any point, if in doubt, compare the behavior of the story with the corresponding story on main available on https://doist.github.io/reactist/

1. Inline

   • Play with the space property in the interactive props story
     • See that spacing between elements works as before.
   • Check the responsive story, switch to canvas mode, and resize the viewport width taking a look at the desktop/tablet responsive size guides at the top.
     • See that spacing between elements continues to change based on viewport size, as before.

2. Stack (similar steps to Inline, but with some more)

   • Play with the space property in the interactive props story
     • See that spacing between elements works as before.
     • Play with alignment, and see that it works as expected and does not impact spacing.
   • Check the responsive story, switch to canvas mode, and resize the viewport width taking a look at the desktop/tablet responsive size guides at the top.
     • See that spacing between elements continues to change based on viewport size, as before.

3. Columns

   • Smoke test comparing between the storybooks of this branch and the one in https://doist.github.io/reactist/
     • pick a couple of columns stories, and see that there's no visual difference on how it first loads. Avoid picking the widths story, as there are expected differences with that one.
   • Check the "Widths story"
     • Change the spacing in the controls in some of those stories and see that the rendered component changes the space between columns accordingly.
     • Columns all have their expected relative size at all times, according to the visible size label.
   • Check the responsive story, switch to canvas mode, and resize the viewport width taking a look at the desktop/tablet responsive size guides at the top.
     • See that spacing between elements continues to change based on viewport size, as before.

Test the library along the app

Run Todoist locally but linked to Reactist locally rather than using the published Reactist package.

• Smoke-test the app. Stacks, Inline and Columns are used so widely, that either you quickly notice several issues, or none at all.
• If in doubt, inspect the elements around some part of the UI that are sure is implemented via Reactist layout components, and inspect the elements and the CSS to the point where you find the gap: CSS applied via Reactist component styling.

PR Checklist

• Added tests for bugs / new features
• Executed npm run validate and made sure no errors / warnings were shown
• Described changes in CHANGELOG.md
• Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
• Updated all static build artifacts (npm run build-all)

Versioning

It looks like this should be a patch release. Not at all. This introduces a new prop in the Box component, so this absolutely needs to be part of a new minor release.

Footnotes

1. In fact, that's how I found out about the issue with the columns described below. ↩

[gnapse]

Update: this seems to be solved now. See below.

I hit an issue trying to use gap to power the columns.

The problem

The issue is accurately and succinctly explained in this StackOverflow question.

I'm trying to use gap to specify gaps between flexed items within my grid system, but running in to a major drawback. It seems that when you're using flex-grow: 0;/flex-shrink: 0; in conjunction with gap and flex-basis values that fill the entire available width (i.e. three columns with flex: 0 0 33.3333%;), the columns overflow their parent container as the gap doesn't account for the fixed width as specified with flex: 0 0 33.3333%.

Similar to box-sizing: border-box;, is there some way to instruct the rendering engine that the gap should be subtracted when determining the width of these columns?

It includes code to reproduce, and I can show here how this manifested in our own columns components:

Loot at the top right area how all the rows that had no column sized as auto suffer from this issue. When all columns in a row have a fraction-based width, it accumulates as many gaps as it has between columns, and that amount of space overflows to the right.

Potential fix

After some investigation, and thanks to insights from an answer in that very StackOverflow page, it turns out that we can "solve" this by switching to use width instead of flex-basis. With a caveat. See the picture below with the same columns story now "fixed":

You can see how having different numbers of gaps in a row can affect how the widths are calculated. If a row has two columns (and therefore 1 gap) the percentages for the width are computed as

(containerWidth - numberOfGaps*gapSize) * columnPercentage

So clearly, the width of a 1/4 column is going to change depending on numberOfGaps.

Why our current non-gap approach works?

Compare the above situation with how it works today:

It works with the code today because it gives us more control over the gap. The way it works today is, each column owns the spacing (it's implemented as padding on the column container element). So when the browser rendering engine computes the size of a column based on its percentage, the spacing is already in it. There's no way for the number of columns to affect the spacing size.

What should we do?

I see 3 options¹:

1. Keep it as shown in the "Potential fix" section above. Some reasons why this may not be as bad as it seems:

   • We barely use fraction-based column widths
   • Even when we do, we often use them alongside auto width columns
   • Even if none of the above applies, we are unlikely to align columns in two consecutive rows in a way that will make the issue evident (as it is made evident in the more artificial example on Storybook).

2. Drop support in Reactist for fraction-based column widths.

   • We barely use fraction-based column widths.

3. We keep the current implementation not based on CSS gap

   • This would apply for columns only. Stack and Inline are doing great with gap.
   • This is what I personally vote for.

Footnotes

1. Unless we find a fix that does what we expect while still using gap ↩

[henningmu]

Let's drop everything we're not using 👍

[gnapse]

Let's drop everything we're not using 👍

@henningmu does this mean you're voting for option 2?

Just to be clear, when I said "we barely use fraction-sized columns", it means that we do use that feature a bit. We'd have to figure out the handful of cases in which we use it (2 or 3 in Todoist, if I recall correctly).

One alternative I can see us adopting is to drop all fractions except "1/2". Having the ability to create a half-and-half layout is something I would not like to lose, and this layout cannot possibly suffer from the described short-coming. I could see me voting for option 2 in that case.

[gnapse]

Update: I may be on to something with this answer. Still not a 100% there, but looking good.

[gnapse]

Another update: it seems to be working! (with a caveat, see below)

Screenshot

Will double-check running Todoist with this, and will switch to ready to review if all goes well on my side.

With a caveat

It breaks part of the behaviour of the Columns's collapseBelow:

On `main`	In this PR

We used to achieve this by having a fixed CSS width: 100% on each column except the ones with width="content". However, to be honest, I think this was always broken in a way. Because on main right now, if you have a column with width="content", it does not auto-expand to the full width when columns collapse into a stack.

Screenshot

So I say that this is not a blocker. I already validated how to fix the only two places where we use <Columns collapseBelow /> in Todoist.

[rfgamaral]

Looks good to me 🚀 Great job 👍

I guess the real test for this will be to update both todoist-web and twist-web with this version, and smoke test it. I can't remember any particular use case from the top of my head, but I remember that sometimes I had to add extra margins/paddings for things to be properly aligned.

[rfgamaral]

💭 Is this comment meant to be kept around?

[gnapse]

Nope. Good catch.

[gnapse]

I guess the real test for this will be to update both todoist-web and twist-web with this version, and smoke test it.

@rfgamaral did you go through the test plan above? It includes a step about running Todoist locally with this version of the library linked locally as well.

I did it myself, but I would not want to merge this until someone else do that too.

[rfgamaral]

@gnapse No, I didn't. I always had trouble linking Reactist to Todoist, not exactly sure why, so I focused on the code only. Let's wait for @scottlovegrove review, otherwise I'll take a second look at a later time, if that's okay with you.