fix(DataList): Padding on first column and alignment for headers#3277
fix(DataList): Padding on first column and alignment for headers#3277LinKCoding wants to merge 12 commits intomainfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit 566a00e ☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3277 +/- ##
==========================================
- Coverage 89.87% 89.03% -0.84%
==========================================
Files 361 236 -125
Lines 5144 4324 -820
Branches 1645 1477 -168
==========================================
- Hits 4623 3850 -773
+ Misses 513 466 -47
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| : sortDirection === 'asc' | ||
| ? 'ascending' | ||
| : 'descending'; | ||
| {selectable && ( |
There was a problem hiding this comment.
removed the fragment b.c. it was interfering with the addition of data-first-col for the first element in the TableHeader
Also doesn't seem necessary
There was a problem hiding this comment.
Not using data-*-col approach anymore, so re-adding is fine, but it's also not necessary from what I can tell
LinKCoding
changed the title
packages/gamut/src/List/utils.ts
Outdated
| import { Children, cloneElement, isValidElement, ReactNode } from 'react'; | ||
|
|
||
| /** | ||
| * Marks the first and last columns with `data-first-col` / `data-last-col` |
There was a problem hiding this comment.
i really dislike cloning children, especially in a component like a DataList where we're already looping though all the columns to render them and there could be alot of them. maybe we you can move this logic into checking the idx of the React.Map in TableHeaderRow?
There was a problem hiding this comment.
FWIW, I did try this approach and it worked well for TableHeaderRow!
But when I went to see how we could use it for List, I ran into an issue again where the children are pretty configurable - i.e. I couldn't just add to the children's attribute without some type of new iteration.
And then after trying another approach with setting context and passing on the idx, the robot confirmed my suspicion that it's just as expensive as cloning.
So instead, I went back to the drawing board and realized that if we can't add padding to the children, maybe we can do that for the parent (row) instead. Since we only want either the first td or th — which seems like it'd translate to just the px of the row itself.
Looks like this works, and I've cleaned up some code that tries to be selective about what child gets padding. But yea, would compare against DataList, DataTable, and List.
|
📬 Published Alpha Packages: |
|
🚀 Styleguide deploy preview ready! Preview URL: https://69b038d84441ef5fe6c25812--gamut-preview.netlify.app |
3 participants
Overview
PR Checklist
Testing Instructions
Don't make me tap the sign.
8padding on the column's textpl: 8and the last element getspr:8(this can be done on one or many DataList)PR Links and Envs