fix(tile): remove support for imgs - #3274
Conversation
|
Preview: https://patternfly-pr-3274.surge.sh A11y report: https://patternfly-pr-3274-coverage.surge.sh
|
mattnolting
left a comment
mattnolting
left a comment
There was a problem hiding this comment.
Just the one comment
| {{#> tile-header tile-header--modifier="pf-m-stacked"}} | ||
| {{#> tile-icon}} | ||
| <img src="/assets/images/pf-logo-small.svg" alt="PatternFly logo"> | ||
| {{#> tile-img-pf}}{{/tile-img-pf}} |
There was a problem hiding this comment.
These can all be
| {{#> tile-img-pf}}{{/tile-img-pf}} | |
| {{> tile-img-pf}} |
| --pf-c-tile__icon--Color: var(--pf-global--Color--200); | ||
| --pf-c-tile--hover__icon--Color: var(--pf-global--primary-color--100); | ||
| --pf-c-tile--m-selected__icon--Color: var(--pf-global--primary-color--100); | ||
| --pf-c-tile--m-disabled__icon--Color: var(--pf-global--disabled-color--100); |
There was a problem hiding this comment.
We should leave this in. If someone sets --pf-c-tile__icon--Color: red;, then it will still be red if it's disabled since we only set the icon color for stacked icons.
There was a problem hiding this comment.
Also need to add back so the stacked icon color is disabled
--pf-c-tile__header--m-stacked__icon--Color: var(--pf-global--disabled-color--100);
|
Just an idea, but this might be a time to try something like where Then we have a theme var for the tile "type" (eg, stacked), and set Then for hover/focus/selected/disabled, we set That way we're only setting 1 var on hover, focus, etc, for the icon color versus needed to set 2 separate vars. That's assuming we don't think we would separate out the icon hover/focus/selected/disabled color between normal and stacked variations, which it doesn't seem like we would. Easier for us and the user to customize. We haven't done that before, but I'm curious what you think @christiemolloy @mattnolting |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
One last thing - the disabled tiles need tabindex="-1" so they aren't focusable, and we should add that to the a11y docs.
| --pf-c-tile__header--m-stacked__icon--Color: var(--pf-global--Color--100); | ||
| --pf-c-tile--hover__header--m-stacked__icon--Color: var(--pf-global--primary-color--100); | ||
| --pf-c-tile--m-selected__header--m-stacked__icon--Color: var(--pf-global--primary-color--100); | ||
| --pf-c-tile--m-disabled__header--m-stacked__icon--Color: var(--pf-global--disabled-color--100); |
There was a problem hiding this comment.
Looks like you need to add this back. It's used but no longer defined:
5 participants
As a follow up to #3229 this PR removes support for images in the tile component and only supports SVG's which take on the font-size property set on the tile-icon.