Skip to content

Vertically align cell contents in DataTable #3843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 31, 2023
Merged

Vertically align cell contents in DataTable #3843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4432ac
Veritcally align cell contents
thyeggman Oct 19, 2023
c312718
Add test
thyeggman Oct 19, 2023
e3b8091
Change styles and add display: flex
thyeggman Oct 19, 2023
223b9dc
Fix test
thyeggman Oct 19, 2023
f000f72
Create popular-cougars-look.md
joshblack Oct 31, 2023
1fae2c7
Merge branch 'main' into thyeggman/align-table-cell
joshblack Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/popular-cougars-look.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Vertically align cell contents in `DataTable`
11 changes: 5 additions & 6 deletions src/DataTable/Table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ const StyledTable = styled.table<React.ComponentPropsWithoutRef<'table'>>`
.TableHeader,
.TableCell {
text-align: start;
display: flex;
align-items: center;
Copy link
Contributor

@mperrotti mperrotti Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about how this might affect the readability of rows where some cells have 1 line of text, and other cells wrap to 2+ lines.

Compare these two tables:
vertically centered rows

flush top rows

Copy link
Contributor Author

@thyeggman thyeggman Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good point. I'm not sure if the best solution is something that involves proper padding if there's a row action available, or increasing the base height of a row when there is no action. I don't think anyone but us is using DataTable at the moment, and our current plan is to truncate row content when it overflows a cell.

Copy link
Member

@dylanatsmith dylanatsmith Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late on this but I think align-items: baseline would solve for that specific issue, though not sure if it would introduce problems with other elements like IconButtons or Labels.

border-bottom: 1px solid ${get('colors.border.default')};
padding: var(--table-cell-padding);
}

.TableHeader[data-cell-align='end'],
Expand Down Expand Up @@ -102,12 +105,6 @@ const StyledTable = styled.table<React.ComponentPropsWithoutRef<'table'>>`
border-bottom-right-radius: var(--table-border-radius);
}

/* TableHeader, TableCell */
.TableCell,
.TableHeader {
padding: var(--table-cell-padding);
}

/**
* Offset padding to make sure type aligns regardless of cell padding
* selection
Expand Down Expand Up @@ -160,6 +157,8 @@ const StyledTable = styled.table<React.ComponentPropsWithoutRef<'table'>>`

/* TableCell */
.TableCell[scope='row'] {
align-items: center;
display: flex;
color: ${get('colors.fg.default')};
font-weight: 600;
}
Expand Down
18 changes: 18 additions & 0 deletions src/DataTable/__tests__/Table.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,24 @@ describe('Table', () => {
)
expect(screen.getByRole('rowheader', {name: 'Cell'})).toBeInTheDocument()
})

it('should vertically align cell contents', () => {
render(
<Table>
<Table.Head>
<Table.Row>
<Table.Header>Column</Table.Header>
</Table.Row>
</Table.Head>
<Table.Body>
<Table.Row>
<Table.Cell>Cell</Table.Cell>
</Table.Row>
</Table.Body>
</Table>,
)
expect(screen.getByRole('cell')).toHaveStyle('align-items: center')
})
})

describe('Table.Skeleton', () => {
Expand Down