Skip to content

Align calculated column widths with cell padding in DetailsList#5769

Merged
dzearing merged 5 commits intomicrosoft:masterfrom
ThomasMichon:cell-padding
Aug 3, 2018
Merged

Align calculated column widths with cell padding in DetailsList#5769
dzearing merged 5 commits intomicrosoft:masterfrom
ThomasMichon:cell-padding

Conversation

@ThomasMichon
Copy link
Copy Markdown
Member

@ThomasMichon ThomasMichon commented Aug 1, 2018

Overview

This change fixes a regression in the alignment of cell padding in DetailsList with the calculated column widths. Specifically, the calculation was not providing enough room for the larger cell padding values, resulting in icon columns experiencing overflow and the whole row being miscalculated.

Details

Added a rowHorizontalWidth and isPaddedMargin to the props and style props of DetailsRow, DetailsHeader, and DetailsRow fields, optional, but provided overall by DetailsList. These are fed into the getStyles calculations, ensuring that everything aligns, and that the values may be overridden if desired.

Changed the paddingLeft styles of cells to be rowHorizontalPadding instead of 12px. This means the left-padding goes from 12px to 8px, bu now there are no more visual bugs.

Microsoft Reviewers: Open in CodeFlow

@cliffkoh
Copy link
Copy Markdown
Contributor

cliffkoh commented Aug 1, 2018

Given that Aditi is out, assigning @dzearing and @kenotron to review this change.

Copy link
Copy Markdown
Contributor

@kenotron kenotron left a comment

Choose a reason for hiding this comment

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

npm run update-snapshots

): IColumn[] {
const { selectionMode, checkboxVisibility, groups } = props;
const outerPadding = DEFAULT_INNER_PADDING;
const outerPadding = DEFAULT_ROW_HORIZONTAL_PADDING * 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not too familiar with this bit here, but since we now pass these in as props, should we then use props.rowHorizontalPadding here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kenotron In DetailsList.base.tsx they are coming from simply importing them from the DetailsRow.styles and not passed by props)))

Copy link
Copy Markdown
Contributor

@Vitalius1 Vitalius1 left a comment

Choose a reason for hiding this comment

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

@ThomasMichon FYI: the 12px padding left you are changing originated from this PR 4857. Make sure you are not causing any visual bugs in that area instead.

@ThomasMichon
Copy link
Copy Markdown
Member Author

@Vitalius1 It would appear that PR #4857 likely caused the visual bugs, by creating this misalignment in the first place. We can have the padding be 12px or 8px, but not both at once!

Copy link
Copy Markdown
Contributor

@Vitalius1 Vitalius1 left a comment

Choose a reason for hiding this comment

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

@ThomasMichon Even though you've consolidated the source of this 2 constants it still feels like we have other ones scattered across all files. In the future would be a good idea to have a file called something like StyleConstants.ts at the root of the DetailsList folder exporting all of them from one place.

@ThomasMichon ThomasMichon force-pushed the cell-padding branch 2 times, most recently from 373102c to 0eac3f5 Compare August 2, 2018 15:31
Copy link
Copy Markdown
Contributor

@kenotron kenotron left a comment

Choose a reason for hiding this comment

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

LGTM

@ThomasMichon
Copy link
Copy Markdown
Member Author

I updated the left padding to 12px to be consistent with intent, while leaving the right padding as 8px. I consolidated into a cellStyleProps with a default value set right now in DetailsRow.styles. I don't want to add a new bundle file for the moment to avoid causing rattle as we propagate the fix.

@kenotron
Copy link
Copy Markdown
Contributor

kenotron commented Aug 2, 2018

The screener's detailslist change seemed to have made it wider... not sure why...

@ThomasMichon
Copy link
Copy Markdown
Member Author

Hm. The column headers being wider is intended, but the GroupedList misalignment is bad. Investigating.

@ThomasMichon
Copy link
Copy Markdown
Member Author

Hey @aditima, I need your sign-off for this, I guess! Also, who approves Screener updates? You'll notice the current versions look better!

@cliffkoh
Copy link
Copy Markdown
Contributor

cliffkoh commented Aug 3, 2018

@betrue-final-final approves screener updates :)
@aditima is OOF and you're normally her backup but since you created the PR, it would need to be @dzearing and @kenotron.

@dzearing dzearing merged commit 80ec6fa into microsoft:master Aug 3, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants