fix(table): updated thead td to th#6689
Conversation
|
Preview: https://patternfly-pr-6689.surge.sh A11y report: https://patternfly-pr-6689-a11y.surge.sh |
33f6778 to
7beb642
Compare
7beb642 to
77fe567
Compare
8dec9b6 to
327813e
Compare
There was a problem hiding this comment.
Looking great ⭐ Just 1 last thing, the Favorites sortable example need an aria-label applied to the th element for the first column where the "favorite all" and "sort" buttons are (could use the screen reader text "Row favorited" from the previous example)
327813e to
5836fc5
Compare
|
Attaching the backstop report - BackstopJS Report.pdf Looks like there may at least be an issue with icons rendering in menu toggles. Here's a toolbar example that's rendering an empty toggle 
|
0e10069 to
68bd944
Compare
|
@thatblindgeye would you mind reviewing this again? Particularly with the last changeset - 68bd944 |
There was a problem hiding this comment.
Left a couple of comments and attaching the backstop report:
table-8.19.24.pdf
Mostly it looks like these two things:
- column widths changed (even when there are no empty headers, actions, or truncated test)
- some
<th>cells are no longer truncated by default
Are those to be expected?
If there are more updates, would you mind attaching visual regressions and leave a comment explaining the errors?
| :where(&) > :is(thead, tbody), | ||
| &.#{$table}__thead { |
There was a problem hiding this comment.
This compiles to
.pf-v6-c-table :where([class*=pf-v6-c-table]),
:where(.pf-v6-c-table) > :is(thead, tbody),
.pf-v6-c-table.pf-v6-c-table__thead
Just want to verify that's correct - particularly the last line? Curious when we have .pf-v6-c-table.pf-v6-c-table__thead if so.
| {{#if toolbar-template--HasToggleGroup}} | ||
| {{#> toolbar-group toolbar-group--IsToggleGroup=true toolbar-group--modifier="pf-m-show-on-xl"}} | ||
| {{> toolbar-toggle}} | ||
| {{> toolbar-toggle menu-toggle-icon='filter'}} |
There was a problem hiding this comment.
Did you mean to leave this in?
| {{> toolbar-toggle menu-toggle-icon='filter'}} | |
| {{> toolbar-toggle}} |
68bd944 to
0d20b06
Compare
There was a problem hiding this comment.
Other than the a11y failures this looks good
0d20b06 to
1efbf1c
Compare
There was a problem hiding this comment.
@mattnolting LGTM, just want to confirm the changes in the backstop report are good.
BackstopJS Report.pdf
Particularly this change on the sortable favorites table example which changes the favorite button from a sorted state to not
| &.pf-m-border-row { | ||
| border-block-end: var(--#{$table}--border-width--base) solid var(--#{$table}--BorderColor); | ||
| } |
There was a problem hiding this comment.
Just want to make sure this is intentional? If so can you give a summary of why? Looks like we'll need a react update for this, too.
|
🎉 This PR is included in version 6.0.0-alpha.227 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #6272
closes #6643
Backstop Report
8/26 Update
Header shift is due to
pf-v6-c-table__cell-emptyand is expectedBackstop report
Favorites example shouldn't be sorted, this diff is expected
This PR creates a partial container for special table cells. The internals of the partials remain unchanged while the wrapping element detects
IsTheadand returnsthif true,tdif false. Empty cells' default screen reader text is set to "Action" (the most common use case) and optionally updated text viatable-cell--text.This shift is due to updating action empty
thtopf-v6-c-table__cell-emptyThis and all other search input shifts are due to button updates unrelated to this PR