components icon indicating copy to clipboard operation
components copied to clipboard

refactor(grid): Replace negative margins with CSS grid gap

Open avinashbot opened this issue 3 years ago • 1 comments

Description

Proof of concept for replacing the negative margin approach in the grid component with the CSS native gap property. Safari 13.x/14.0 doesn't support gap on flex, so it's implemented as a "one-dimensional" CSS grid. Push/pull continues to work, but I've also marked it as deprecated in the interface - we mention this in the gridDefinition documentation, but don't mark it that way in the interface. This way, IDEs will mark those properties as deprecated inline.

The big two benefits to dropping negative margins are:

  • no more scrollbars/overflow under the document or scroll containers without padding,
  • no more pointer-events hacks because of negative margins extending outside the component,
  • more precise JS space calculations (e.g. for ResizeObserver or getBoundingClientRect)

Unfortunately, due to column layout's design, the negative margins there will have to remain, at least for now, so I don't blow out the scope of this refactor into a redesign. The borders visually extend outside the borders of the component, and the only way to do that is negative margins.

How has this been tested?

Nothing is supposed to change, so these are tested through unit tests, but more importantly through visual regression tests.

Documentation changes

[Do the changes include any API documentation changes?]

  • [x] Yes, this change contains documentation changes.
    • Somewhat. Added @deprecated to push/pull but this doesn't show up on the website.

Related Links

[Attach any related links/pull request for this change]

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • [x] Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • [x] Changes do not include unsupported browser features, see CONTRIBUTING.md.
    • Ha. Goodbye, IE11!
  • [ ] Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • [ ] Changes are covered with new/existing unit tests?
  • [ ] Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

avinashbot avatar Aug 11 '22 12:08 avinashbot

Codecov Report

Base: 92.59% // Head: 92.59% // Decreases project coverage by -0.00% :warning:

Coverage data is based on head (8c39895) compared to base (8fc8788). Patch coverage: 93.75% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   92.59%   92.59%   -0.01%     
==========================================
  Files         558      558              
  Lines       15904    15914      +10     
  Branches     4364     4369       +5     
==========================================
+ Hits        14726    14735       +9     
- Misses       1097     1098       +1     
  Partials       81       81              
Impacted Files Coverage Δ
src/attribute-editor/row.tsx 100.00% <ø> (ø)
src/grid/internal.tsx 97.67% <91.66%> (-2.33%) :arrow_down:
src/column-layout/internal.tsx 100.00% <100.00%> (ø)
src/test-utils/dom/grid/index.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Aug 11 '22 12:08 codecov[bot]

Using a twelve column grid results in the gaps always being rendered, and doesn't let the grid container shrink below a certain minimum width. It looks like a fundamental limitation with the way I'm using CSS grid here. Ideally, of course, this could all be done with flex gap to preserve the original behavior, but... *shakes fist at Safari 14.0*

Welp. Can't say I haven't tried. o7

avinashbot avatar Oct 27 '22 16:10 avinashbot