fix(card): remove padding/margin on clickable cards action element#7928
Conversation
|
Preview: https://pf-pr-7928.surge.sh A11y report: https://pf-pr-7928-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
LGTM, just thinking that wwe probably want to make an update for when a selectable input is also hidden, like in the React card as tiles examples. @mcoker
| &:has(button, a) { | ||
| padding: 0; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
If I'm reading the issue correctly, this should be scoped just to the pf-m-clickable card. As is, this is removing padding and margin for any card actions that contain a link or button - which is probably all or most of them, right?
Yep, any time the actions element is in the DOM but invisible, it shouldn't impact the layout of the card. |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM! Just one little nit so it would work better with nested cards.
| &.pf-m-clickable:not(.pf-m-selectable), | ||
| &:has(input.#{$pf-prefix}u-screen-reader) { |
There was a problem hiding this comment.
You'd probably want a more narrow scope on the input to be able to handle nested cards. > .#{$card}__selectable-actions is optional but I'd include it because who knows if someone has some other thing in their card actions that could trigger this style change.
| &.pf-m-clickable:not(.pf-m-selectable), | |
| &:has(input.#{$pf-prefix}u-screen-reader) { | |
| &.pf-m-clickable:not(.pf-m-selectable), | |
| &:has(> .#{$card}__header > .#{$card}__actions > .#{$card}__selectable-actions input.#{$pf-prefix}u-screen-reader) { |
It's probably more efficient to write it this way though (not tested) since targeting the input only needs to start at the __actions container?
| &.pf-m-clickable:not(.pf-m-selectable), | |
| &:has(input.#{$pf-prefix}u-screen-reader) { | |
| @at-root .#{$card}__actions:has(> .#{$card}__selectable-actions input.#{$pf-prefix}u-screen-reader), | |
| &.pf-m-clickable:not(.pf-m-selectable) { |
|
@srambach @wise-king-sullyman I pushed the update from my comment about updating the scope in this commit fbe2de1 Also it removes the To verify that it works with tiles, I copied/pasted the HTML from the react cards as tiles example into the core preview with these changes. |
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
Visual regressions on card looked good. Just one suggestion to add a comment if you like.
| // stylelint-enable selector-max-class | ||
|
|
||
| // when the whole card is clickable and the card actions are invisible, remove the padding/margin so it doesn't take up space | ||
| @at-root .#{$card}__actions:has(> .#{$card}__selectable-actions input.#{$pf-prefix}screen-reader), |
There was a problem hiding this comment.
My only comment would be maybe it's worth a comment here to explain what this selector is aiming to do since it's pretty obscure and really only in react.
|
@srambach thanks for running visual regressions!! |
|
🎉 This PR is included in version 6.5.0-prerelease.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…atternfly#7928) * fix(card): remove padding/margin on clickable cards action element * fix(card): only remove padding on actionable only or hidden input cards * chore: update selector --------- Co-authored-by: mcoker <mcoker@redhat.com>
5 participants
Closes #7899