Conversation
6f0f95d to
8f944ef
Compare
|
Preview: https://patternfly-pr-6294.surge.sh A11y report: https://patternfly-pr-6294-a11y.surge.sh |
1463bfb to
7602350
Compare
7602350 to
45dc280
Compare
45dc280 to
630f6b3
Compare
630f6b3 to
a01f949
Compare
There was a problem hiding this comment.
LGTM! Left some comments as I was looking at the code but nothing that needs action, feel free to address later if you think any of the comments are relevant.
| padding-block-end: var(--#{$table}__button--PaddingBottom); | ||
| padding-inline-start: var(--#{$table}__button--PaddingLeft); | ||
| padding-inline-end: var(--#{$table}__button--PaddingRight); | ||
| margin-inline-start: calc(var(--#{$table}__button--PaddingLeft) * -1); |
There was a problem hiding this comment.
Bold strategy, but let's see how it plays out! ☄️
| thead & { | ||
| vertical-align: bottom; | ||
| } | ||
|
|
||
| tbody & { | ||
| vertical-align: top; | ||
| } |
There was a problem hiding this comment.
just a thought, but you could probably add vertical-align: top as the base style under the display declaration and just change that in thead?
| --#{$check}__input--TranslateY: var(--#{$table}__thead__check--input--TranslateY); | ||
| // TODO: remove label wrapper at breaking change | ||
| label { | ||
| display: contents; |
There was a problem hiding this comment.
I wonder if this will cause an a11y issue since last I read, display: contents still isn't a great thing for elements with semantic value specifically around remaining in the a11y tree properly.
| margin-inline-start: var(--#{$table}__favorite--c-button--MarginLeft); | ||
| margin-inline-end: var(--#{$table}__favorite--c-button--MarginRight); | ||
|
|
||
| @at-root .pf-m-favorited#{&} { |
There was a problem hiding this comment.
just a nit but since this isn't nested, I think you could just write it normally?
| @at-root .pf-m-favorited#{&} { | |
| &.pf-m-favorited { |
| > td, | ||
| > th { |
There was a problem hiding this comment.
Do we need to add the class selector here?
| @import 'themes/dark/table'; | ||
|
|
||
| @include pf-v5-theme-dark { | ||
| @include pf-v5-theme-dark-table; |
There was a problem hiding this comment.
you can take these out of all the stylesheets. maybe that's part of some future work?
|
🎉 This PR is included in version 6.0.0-alpha.89 🎉 The release is available on: Your semantic-release bot 📦🚀 |
relates to #5728
This PR updates padding, margin, and fitting of table elements.
Preview: https://patternfly-pr-6294.surge.sh/components/table