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 16 commits intomainfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit 80a2fdf ☁️ Nx Cloud last updated this comment at |
|
| : 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.
dreamwasp
left a comment
dreamwasp
left a comment
There was a problem hiding this comment.
lgtm! if you have time could you remove the red BG on this List example? ik it is unrelated and literally probably something i did
i
|
🚀 Styleguide deploy preview ready! Preview URL: https://69b86a09c7047730b594d3d0--gamut-preview.netlify.app |
|
📬 Published Alpha Packages: |
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