fix(card): remove unused modifier and old logo from demo#6972
fix(card): remove unused modifier and old logo from demo#6972mcoker merged 4 commits intopatternfly:v6from
Conversation
|
Preview: https://patternfly-pr-6972.surge.sh A11y report: https://patternfly-pr-6972-a11y.surge.sh |
There was a problem hiding this comment.
Awesome!! Here is a visual regression report showing what changed to any examples/demos with "card" in the name from this PR:
BackstopJS Report.pdf
BackstopJS Report-dark.pdf - this one I ran in reverse order, so the images are out of order 😅
One little thing is the logo file used in https://patternfly-pr-6972.surge.sh/components/card/html/with-only-image-in-head/ (and other examples) isn't ideal in dark theme because the text doesn't change colors.
We have a variation of that image that uses PF's text-color tokens for the "Patternfly" text that we use in the masthead. We could use that in these card demos, too. https://github.com/patternfly/patternfly/blob/v6/src/patternfly/components/Masthead/masthead-image.hbs
The only issue with masthead-image.hbs is it relies on masthead--id as a handlebars parameter either on or as an ancestor of the include for this partial ({{> masthead-image}}). So if you were to use that in these card examples, you'd have to specify masthead--id somewhere in those examples for that SVG's ID to work properly and not generate duplicate IDs on the page. There are a bunch of ways you could do it, but the simplest for the time being would probably be - if the {{#> card ...}} examples that would include masthead-image.hbs have a card--id="..." on the outer {{#> card}}, then you could replace the logos in those cards with this: {{> masthead-image masthead--id=card--id}}. There is also a hard-coded height attribute on the svg in masthead-image.hbs - if that doesn't work in the card examples, you could do something like change height="37px" to height="{{ternary masthead-image--height masthead-image--height "37px"}}", then you could pass a custom height via {{> masthead-image masthead--id=card--id masthead-image--height="1234px"}}
|
@rebeccaalpert left a comment but LGTM! Your comments about running the screenshots job and auditing the other images in the assets folder would be great, though I agree this PR is already pretty big and it would be good to do that in a follow-up. If you wanted to work on the stuff in my comment above, that could be a follow-up, too. Just lemme know how you'd prefer to handle it! |
|
Happy to do the masthead-image.hbs sub in this PR. If it's ok, I'll do the screenshots and looking through the other images separately so it's less of a pain for folks to review. |
masthead-image has better compatibility with dark mode
|
The swap for masthead-image should be all set! |
|
Follow-on for additional images is here: #6976. |
|
🎉 This PR is included in version 6.0.0-alpha.212 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #6249
I fixed the Card demo as outlined in the issue. I also noticed the examples for Cards also had old logos - I took a look at fixing those as well here. I updated Primary-Detail and the Card view patterns as well since it allowed me to remove an old logo image.
Since I modified some demos, we'll have to update some screenshots I think, but I'm not sure if I do those as part of a PR or not, so I didn't. Please let me know if I should!
There are some other old images in the assets folder that look like they are in use - I would love to go through and remove/update those too, but I feel like this is already large.