fix(Table): remove duplicate bottom border on mobile collapsed expandable rows#6698
Conversation
|
Preview: https://patternfly-pr-6698.surge.sh A11y report: https://patternfly-pr-6698-a11y.surge.sh |
mattnolting
left a comment
mattnolting
left a comment
There was a problem hiding this comment.
This update removes more borders than I believe are intended.
Before:
After:
|
|
||
| // * Table tr responsive | ||
| --#{$table}__tr--responsive--border-width--base: var(--pf-t--global--border--width--divider--default); | ||
| --#{$table}__tr--responsive--border-width--base: 0; |
There was a problem hiding this comment.
| --#{$table}__tr--responsive--border-width--base: 0; | |
| --#{$table}__tr--responsive--border-width--base: var(--pf-t--global--border--width--divider--default); |
|
Suggestions above are correct that the previous code changed more than the intended target, but the proposed solution looks to revert the original problem this PR aims to solve. The issue is that:
There are several rules similar to this case, but all focus on the tr whereas we need to focus on the class on the parent |
| // Remove border on tr inside non-expanded tbody in of expandable tables | ||
| &.pf-m-expandable tbody:not(.pf-m-expanded) tr { | ||
| border-block-end: 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
This does what we want, but something like this would be more straightforward. Disable all <tr> border-block-end. Initially, this logic applied to a mix and match approach to expandable table (tbody.expandable next to a regular row). However, React's guidance places all expandable table content in a loop, within a Tbody. I believe it's safe to take this approach.
Add this variable to :root (.#{$table}[class*="pf-m-grid"] in this case) to avoid potential conflicts
--#{$table}__tbody--responsive--m-expandable--BorderBlockEndWidth: var(--pf-t--global--border--width--divider--default);
&.pf-m-expandable {
--#{$table}__tr--BorderBlockEndWidth: 0; // nested table rows have no border
.#{$table}__tbody {
border-block-end: var(--#{$table}__tbody--responsive--border-width) solid var(--#{$table}--responsive--BorderColor);
}
}
You should then be able to remove lines 190-199 and lines 201-212, but because pf-v-table is very complex, we should run a visual regression test before merging.
There was a problem hiding this comment.
@mattnolting I've made these changes but noticed I also needed to specifically target .#{$table}__tbody.pf-m-expanded to apply as needed so added that specific rule to what you laid out above 👍
mattnolting
left a comment
mattnolting
left a comment
There was a problem hiding this comment.
Wonderful. Last thing, I believe you can remove lines 299 - 300. Looks like the padding between rows is uneven. @kaylachumley Can you confirm?
Before
After
|
yep! That looks better. can confirm to remove rows 299-300! @mattnolting |
|
Thanks @mattnolting for the help! Just removed lines 299-300 🤞
|
|
🎉 This PR is included in version 6.0.0-alpha.179 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
Closes #6556
This PR removes the bottom border on collapsed expandable rows on mobile, which is causes a double border to appear.