Doist / reactist

Open source React components made with ❤️ by Doist
http://doist.github.io/reactist
MIT License
248 stars 22 forks source link

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

Closed gnapse closed 1 year ago

gnapse commented 1 year ago

Short description

Introduce support for the CSS gap property.

References

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[^1], but a second pair of eyes would not hurt.

[^1]: In fact, that's how I found out about the issue with the columns described below.

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
      • [x] 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.
      • [x] 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
      • [x] See that spacing between elements works as before.
      • [x] 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.
      • [x] 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/
      • [x] 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"
      • [x] Change the spacing in the controls in some of those stories and see that the rendered component changes the space between columns accordingly.
      • [x] 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.
      • [x] 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.

PR Checklist

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.

gnapse commented 1 year ago

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](https://stackoverflow.com/q/72116170). > 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: ![CleanShot 2023-01-06 at 12 12 05@2x](https://user-images.githubusercontent.com/15199/211043885-d15ff9d1-446e-48ba-b7f3-2dbc5b94050e.png) 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](https://stackoverflow.com/a/72117197), 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": ![CleanShot 2023-01-06 at 12 26 45@2x](https://user-images.githubusercontent.com/15199/211044680-5cf858e7-6c4e-4c19-ad9c-29e6da8c706f.png) 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: ![CleanShot 2023-01-06 at 12 41 41@2x](https://user-images.githubusercontent.com/15199/211046176-b8646b3d-f864-4aab-9814-8efa227cb8ed.png) 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]: 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.

[^1]: Unless we find a fix that does what we expect while still using gap

henningmu commented 1 year ago

Let's drop everything we're not using 👍

gnapse commented 1 year ago

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 commented 1 year ago

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

gnapse commented 1 year ago

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

Screenshot image

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
image image

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 image

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.

gnapse commented 1 year ago

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.