components
components copied to clipboard
refactor(grid): Replace negative margins with CSS grid gap
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
@deprecatedto push/pull but this doesn't show up on the website.
- Somewhat. Added
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
- [x] If the code handles URLs: all URLs are validated through the
checkSafeUrlfunction.
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.
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.
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