fix(table): removed first/last child padding, added inset by mattnolting · Pull Request #7050 · patternfly/patternfly

mattnolting · 2024-09-09T13:42:50Z

Updated Table Iso Backstop Report
Updated Full Backstop Report

Examples: https://patternfly-pr-7050.surge.sh/components/table

TLDR:

Floating :first/:last-child pseudos result in a huge number of selector matching attempts. Attaching these pseudos to a selector results in a massive reduction in matching attempts.

The cell padding and button fitting updates are the result of a somewhat hidden bug. thead row heights are affected erratically by using negative margining on table__button cells. See below:

Before

After

Table :first/:last-child css declarations are relics that should be standardized. As per a recent conversation w/ @lboehling, :first/:last-child th/td padding definitions can/should be simplified, while inset updates should be optional.

~~Also, toggles, checkboxes, radios, actions, etc do not need special spacing.~~

These need to be updated

Performance impact

This PR

Reduces match attempts by 4,361,984
Improves performance by 2%
Reduces blocking time by 50ms

Before

After

Before

After

Backstop report

patternfly-build · 2024-09-09T13:45:18Z

Preview: https://patternfly-pr-7050.surge.sh

A11y report: https://patternfly-pr-7050-a11y.surge.sh

mcoker

I'm seeing slightly different results in the backstop report - backstop-first-last-child-table.pdf.

At a glance, it looks like expandable toggles are no longer aligned with the row text? https://patternfly-pr-7050.surge.sh/components/table#expandable

cc @lboehling, would you mind looking at the report or table examples and see if this looks as you'd expect?

mattnolting · 2024-09-20T20:02:59Z

TLDR:

Line height has been standardized to create consistent row height. Currently, rows without actionable elements are shorter than rows with actionable elements. Table sort buttons are shorter than plain buttons (row toggles) which creates a presentational difference, especially notable when a thead toggle button is adjacent to a thead sort button. Because line-height is defined as 1.5, thead block height resolves to 18px, instead of 21px, which is the default for buttons or menu-toggles. It also results in vertical misalignment as some elements may that may not be as tall as others. In order to correct this, a standardized line-height of table--LineHeight * table--FontSize(not th/td LineHeight * th/td FontSize) has been applied to all cells. The result is a consistent block height that is independent of font-size.

@lboehling @kaylachumley @andrew-ronaldson Can you take a look at Backstop report?

What changed

caption inline-padding has been updated to match th/td inline-padding
line-height has been standardized to create consistent row height.
First/last child table cells have been standardized to match menu-toggle padding
radio/checkbox alignment has been corrected

Help

Checkboxes

Radio

Expandable

Compact

Compound expansion

Favorites

Compact

Borderless

Compound expansion

Tree table

Rather than negative margins on sort buttons, table__action cells have been standardized to exclude top padding (in thead cells) and bottom padding (in tbody cells). Margin top has been added to thead sort buttons and bottom margin has been added to tbody sort buttons to retain a common default height of 53px when standard th/td content is absent.

Specific padding variables __toggle, __check, __action , __draggable, __favorite have been abstracted to --#{$table}--cell--action--PaddingBlockStart/End: var(--pf-t--global--spacer--control--vertical--plain); and --#{$table}--cell--action--PaddingInlineStart/End: var(--pf-t--global--spacer--action--horizontal--plain--default);

Mobile presentation has been simplified and has changed slightly as a result

padding-block-start/end is consistent

srambach

I haven't made it all the way through, but ran regression tests on the table examples. I'm seeing some shifts in all different directions and really can't discern what's correct.

I believe the line height change causes some to move up, some down. Spot checks on the desktop width show they are 21px with 16px top and bottom, so that's good.

Mobile view seems to show increased height; is that also intended? e.g. pf-core__components_table_html_sortable-example_0_document_0_mobile.png

Left-right shifts seem to be a difference in padding - I can't actually tell from the issue description - what's the desired padding?

A couple of other things I noticed:
/components/table/html/borderless-expandable-example has some expanded content that lost it's left padding

/components/table/html/borderless-with-compound-expansion-example the kebabs no longer align horizontally between the main table and expanded section

src/patternfly/components/Button/button.scss

src/patternfly/components/Table/table-grid.scss

src/patternfly/components/Table/table-tr.hbs

mcoker · 2024-09-25T21:37:12Z

Backstop report PDF - https://drive.google.com/file/d/1KK0zfG0OJPquXKql5AhnLY8pKRWMJWL-/view?usp=sharing
Full report - https://drive.google.com/file/d/13jjqKLZjH9kVlSKXFDVRui9C7GLVRbQu/view?usp=sharing

You'll need to download, unzip, then open backstop_data -> html_report -> index.html

cc @srambach

src/patternfly/components/Table/table-grid.scss

mattnolting · 2024-09-26T20:47:11Z

@mcoker @srambach @andrew-ronaldson @lboehling @kaylachumley Updated Backstop report

…ng modifications

mattnolting · 2024-09-27T16:01:11Z

Updated Table Iso Backstop Report
Updated Full Backstop Report

mattnolting · 2024-09-27T16:24:34Z

src/patternfly/components/Table/table-grid.scss

-  --#{$table}--cell--PaddingBlockStart: var(--#{$table}--m-grid--cell--PaddingBlockStart);
-  --#{$table}--cell--PaddingInlineEnd: var(--#{$table}--m-grid--cell--PaddingInlineEnd);
-  --#{$table}--cell--PaddingBlockEnd: var(--#{$table}--m-grid--cell--PaddingBlockEnd);
-  --#{$table}--cell--PaddingInlineStart: var(--#{$table}--m-grid--cell--PaddingInlineStart);


--#{$table}--cell--PaddingBlockStart

--#{$table}--cell--PaddingInlineEnd

--#{$table}--cell--PaddingBlockEnd

--#{$table}--cell--PaddingInlineStart

These values are being assigned on lines 141-145 rather than variable chaining. Controlling the chained variables, especially when getting further into modifiers like m-compact, m-clickable, m-selectable, is overly complicated and very fragile. Standard assignment provides clarity, prevents unintended value inheritance, enhances readability, and avoids forced reflows.

.#{$table}__th, .#{$table}__td { padding-block-start: var(--#{$table}--m-grid--cell--PaddingBlockStart); padding-block-end: var(--#{$table}--m-grid--cell--PaddingBlockEnd); padding-inline-start: var(--#{$table}--m-grid--cell--PaddingInlineStart); padding-inline-end: var(--#{$table}--m-grid--cell--PaddingInlineEnd);

mattnolting · 2024-09-27T16:38:24Z

src/patternfly/components/Table/table-grid.scss

-  --#{$table}-td--responsive--GridColumnGap: var(--pf-t--global--spacer--md);
-  --#{$table}--cell--responsive--PaddingBlockStart: var(--pf-t--global--spacer--md);
-  --#{$table}--cell--responsive--PaddingBlockEnd: var(--pf-t--global--spacer--sm);
-  --#{$table}--cell--first-child--responsive--PaddingBlockStart: var(--pf-t--global--spacer--sm);


--#{$table}--cell--first-child--responsive--PaddingBlockStart is no longer necessary as padding is being redefined without targeting first/last children in the grid layout.

mattnolting · 2024-09-27T16:38:54Z

src/patternfly/components/Table/table-grid.scss

-  --#{$table}--m-compact__check--responsive--MarginBlockStart: #{pf-size-prem(7px)};
-  --#{$table}--m-compact__action--responsive--MarginBlockStart: calc(var(--pf-t--global--spacer--xs) * -1);
-  --#{$table}--m-compact__action--responsive--MarginBlockEnd: calc(var(--pf-t--global--spacer--xs) * -1);
+  --#{$table}--m-compact__check--responsive--MarginBlockStart: var(--pf-t--global--spacer--sm);


This can be removed as it's no longer used or necessary.

mattnolting · 2024-09-27T16:42:09Z

src/patternfly/components/Table/table-grid.scss

+  --#{$table}--m-grid__check--MarginInlineStart: var(--#{$table}__tbody--responsive--border-width--base);
+  --#{$table}--m-grid__check--PaddingBlockStart: var(--pf-t--global--spacer--sm);


All other variables including --#{$table}--responsive should be updated to --#{$table}--m-grid as they are each others equivalent, and using them interchangeably is confusing and adds complexity.

mattnolting · 2024-09-27T16:44:32Z

src/patternfly/components/Table/table-grid.scss

+  // * Table caption selected
+  --#{$table}--m-grid__caption--PaddingInlineEnd: var(--pf-t--global--spacer--lg);
+  --#{$table}--m-grid__caption--PaddingInlineStart: var(--pf-t--global--spacer--lg);
+


Table caption padding needs to match table cell padding. These variables do no exist currently and need to be applied with this update.

mattnolting · 2024-09-27T16:46:17Z

src/patternfly/components/Table/table-grid.scss

-  --#{$table}--m-compact__tr__td--responsive--PaddingBlockEnd: var(--pf-t--global--spacer--xs);
-  --#{$table}--m-compact__check--responsive--MarginBlockStart: #{pf-size-prem(7px)};
-  --#{$table}--m-compact__action--responsive--MarginBlockStart: calc(var(--pf-t--global--spacer--xs) * -1);
-  --#{$table}--m-compact__action--responsive--MarginBlockEnd: calc(var(--pf-t--global--spacer--xs) * -1);


These variables are no longer necessary as they are now part of the standardized cell--actions.

mattnolting · 2024-09-27T17:59:45Z

src/patternfly/components/Table/table.scss

+  // * Table cell action
+  --#{$table}--cell--action--PaddingBlockStart: var(--pf-t--global--spacer--sm);
+  --#{$table}--cell--action--PaddingBlockEnd: var(--pf-t--global--spacer--sm);
+  --#{$table}--cell--action--PaddingInlineStart: var(--pf-t--global--spacer--sm);
+  --#{$table}--cell--action--PaddingInlineEnd: var(--pf-t--global--spacer--sm);


--#{$table}__action has been abstracted to --#{$table}--cell--action and applied to all cells that contain interactive elements.

mattnolting · 2024-09-27T18:00:57Z

src/patternfly/components/Table/table.scss

-  --#{$table}--cell--PaddingBlockStart: var(--#{$table}__toggle--PaddingBlockStart);
-  --#{$table}--cell--PaddingBlockEnd: var(--#{$table}__toggle--PaddingBlockEnd);
-  --#{$table}--cell--PaddingInlineStart: var(--#{$table}__toggle--PaddingInlineStart);
-  --#{$table}--cell--PaddingInlineEnd: var(--#{$table}__toggle--PaddingInlineEnd);
-


Toggle no longer needs a specific padding as it belongs to the .#{$table}--cell--action group.

mattnolting · 2024-09-27T18:02:37Z

src/patternfly/components/Table/table.scss

-  --#{$table}--cell--PaddingInlineStart: var(--#{$table}__check--PaddingInlineStart);
-  --#{$table}--cell--PaddingInlineEnd: var(--#{$table}__check--PaddingInlineEnd);
-
-  vertical-align: top;
-
-  .#{$check} {
-    --#{$check}__label--FontSize: var(--#{$table}--cell--FontSize);
-    --#{$check}__label--LineHeight: var(--#{$table}--cell--LineHeight);
-  }
-
+  .#{$check},
  .#{$radio} {
-    --#{$radio}__label--FontSize: var(--#{$table}--cell--FontSize);
-    --#{$radio}__label--LineHeight: var(--#{$table}--cell--LineHeight);
-  }
-
-  thead & {
-    vertical-align: bottom;
-  }
-
-  .#{$check}.pf-m-standalone,
-  .#{$radio}.pf-m-standalone {
-    thead & {
-      display: table-cell;
-      height: auto;
-      line-height: 1;
-      vertical-align: baseline;
-    }


Special styling for __check is no longer necessary as its height has been standardized to table-_FontSize * table__LineHeight or 21px resulting in vertical alignment with all other elements.

mattnolting · 2024-09-27T18:04:00Z

src/patternfly/components/Table/table.scss

+  .#{$table}__thead & {
+    padding-block-start: 0;
+    padding-block-end: var(--#{$table}--cell--action--PaddingBlockEnd);
+  }


For thead .#{$table}--cell--action cells, only bottom padding needs to be applied in order to create consistent vertical alignment, as thead is set to vertical-align: bottom.

mattnolting · 2024-09-27T18:04:36Z

src/patternfly/components/Table/table.scss

+  .#{$table}__tbody & {
+    padding-block-start: var(--#{$table}--cell--action--PaddingBlockStart);
+    padding-block-end: 0;
+  }


For tbody .#{$table}--cell--action cells, only bottom padding needs to be applied in order to create consistent vertical alignment, as tbody is set to vertical-align: top.

mattnolting requested review from andrew-ronaldson, lboehling, mcoker and thatblindgeye September 9, 2024 14:19

mcoker reviewed Sep 12, 2024

View reviewed changes

mattnolting force-pushed the fix-table-first-last-child branch 4 times, most recently from d7689aa to 8c8e486 Compare September 20, 2024 17:55

mattnolting force-pushed the fix-table-first-last-child branch from 8c8e486 to c4eff46 Compare September 20, 2024 20:03

srambach reviewed Sep 24, 2024

View reviewed changes

mattnolting force-pushed the fix-table-first-last-child branch from e913423 to e085400 Compare September 24, 2024 14:47

mattnolting requested a review from mcoker September 24, 2024 18:58

mattnolting force-pushed the fix-table-first-last-child branch from 15ef767 to 4d6585d Compare September 24, 2024 19:23

mcoker mentioned this pull request Sep 25, 2024

Bug - Table - Applying info prop causes Th misalignment #7112

Closed

mattnolting force-pushed the fix-table-first-last-child branch 2 times, most recently from b826b1b to 6dfc8ee Compare September 26, 2024 17:06

srambach reviewed Sep 26, 2024

View reviewed changes

src/patternfly/components/Table/table-grid.scss Outdated Show resolved Hide resolved

mattnolting force-pushed the fix-table-first-last-child branch 2 times, most recently from 153ebe8 to a648b9c Compare September 26, 2024 20:11

mattnolting added 6 commits September 26, 2024 18:20

fix(table): removed first/last child padding, added inset

f69d1ac

fix(table): standardized special cell inline padding

48b76fc

fix(table-th-td): updated first/last child padding, action cell fitti…

a4145a0

…ng modifications

fix(table): simplified updates

a5bf780

fix(table): simplified updates, removed unnecessary updates

dafa16a

fix(table): updated button icon line-height to perfectly align

6e878ee

fix(table): updated table cell configuration

31b29de

mattnolting force-pushed the fix-table-first-last-child branch from a648b9c to 31b29de Compare September 26, 2024 22:21

mattnolting added 2 commits September 26, 2024 18:55

fix(table): updated font-weight token

b72ffb0

fix(table): refined mobile padding

be54e81

mattnolting commented Sep 27, 2024

View reviewed changes

kmcfaul closed this Oct 8, 2024

mattnolting mentioned this pull request Nov 20, 2024

Bug - Table - misalignment when only some headers are sortable #7220

Closed

3 tasks

mattnolting mentioned this pull request Dec 4, 2024

fix(table): isolated sort button negative margins to thead #7246

Merged

evwilkin mentioned this pull request Aug 4, 2025

fix(table): fix toggle vertical alignment in thead #7701

Closed

		--#{$table}--m-grid__check--MarginInlineStart: var(--#{$table}__tbody--responsive--border-width--base);
		--#{$table}--m-grid__check--PaddingBlockStart: var(--pf-t--global--spacer--sm);

Conversation

mattnolting commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TLDR:

Performance impact

Uh oh!

patternfly-build commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting commented Sep 20, 2024

TLDR:

What changed

Mobile presentation has been simplified and has changed slightly as a result

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcoker commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mattnolting commented Sep 26, 2024

Uh oh!

mattnolting commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattnolting Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattnolting commented Sep 9, 2024 •

edited

Loading

patternfly-build commented Sep 9, 2024 •

edited

Loading

mcoker commented Sep 25, 2024 •

edited

Loading

mattnolting commented Sep 27, 2024 •

edited

Loading

mattnolting Sep 27, 2024 •

edited

Loading