fix(table): fix toggle vertical alignment#7722
Conversation
|
Preview: https://pf-pr-7722.surge.sh A11y report: https://pf-pr-7722-a11y.surge.sh |
|
When I compare this demo in your PR to the equivalent demo in pf-core-staging, the alignment doesn't appear improved in your PR. Is this updated spacing what you'd expect in your PR? |
evwilkin
left a comment
evwilkin
left a comment
There was a problem hiding this comment.
LGTM - thanks for the clear description of changes & inline notes for future breaking changes 👏
|
@nicolethoen thanks! The vertical alignment should have been good, but there was a bug in the button width causing a horizontal alignment issue. Would you mind giving it another look? |
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
Looks good now - regression test report (for tables) at https://drive.google.com/file/d/1N3X-Q7P4fSbSnA9WkoxpahmKh-D0Zbe2/view?usp=sharing just shows expected differences
* fix(table): fix toggle vertical alignment * chore(table): fix lh for column help button * chore(table): fix thead expand button min-width
|
🎉 This PR is included in version 6.3.0-prerelease.47 🎉 The release is available on: Your semantic-release bot 📦🚀 |
7 participants
fixes #7699
The general stragy in this PR:
Remove the custom padding that is on the
.#{$table}__th.#{$table}__toggleelement (the<th>that wraps the toggle button in<thead>), so that the padding matches the adjacent<th>cellspatternfly/src/patternfly/components/Table/table.scss
Lines 24 to 25 in aded963
Assign negative block margins to the button that are equal to the button's padding, so that only the button's content (the text/icon) takes up space in the cell. This should match other stuff in adjacent
<th>cells since<th>cells either have text or the.#{$table}__buttonelement, which also uses a negative block margin to make it align with adjacent cells.patternfly/src/patternfly/components/Table/table.scss
Lines 839 to 842 in aded963
Give the button a line-height of
1lh, which will make its line-height the computed line-height of its parent. This handles thefont-sizediscrepancy between a plain button (font-size: 14px) and the<th>cells (font-size: 12px). Previously the text box in the plain button was 21px (14px* 1.5lh), now it's 18px (12px* 1.5lh), which matches adjacent cells. The icon is still technically bigger but it doesn't take up more space than smaller text around it.patternfly/src/patternfly/components/Table/table.scss
Line 843 in aded963
Applied the same line-height fix as abive to the column sort help button because it was causing the cell it's in to be slightly misaligned with adjacent cells.
patternfly/src/patternfly/components/Table/table.scss
Lines 1010 to 1012 in 3ba39e5
I tested the following
.pf-m-compact. For this you need to be make sure.pf-m-smallis on the toggle button (reduces the padding) since that variant is used with compact tables. Looks like the [the change to use pf-m-small in react(https://github.com/fix(table): add pf-m-small to expandable toggle on small tables patternfly-react#11947) is going in with the patch release. It shouldn't matter what size the button is though, it's always offset by its own padding, so it should work with whatever padding is on the button.