Make the header and footer patterns text visible on all themes #7524
Conversation
|
The release ZIP for this PR is accessible via: |
rubikuserbot
requested review from
a team and
Aljullu
and removed request for
a team
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
|
Size Change: 0 B Total Size: 1000 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
I wonder if a better approach would be to remove all colors from the patterns and let them "inherit" the ones from the theme. That will ensure the patterns work in all themes, with any color, and maybe it would allow us to only have one variant of them instead of having dark/light ones.
Personally, I think that's a better approach than having one with white background and another one with black background. Specially because with TT3 many style variations have peculiar backgrounds like images and gradients so white/black headers/footers might not work well.
Do you think that's doable, @albarin?
@Aljullu Yes, I agree, I've been testing the approach you mentioned and it works ok with multiple themes and style variations. Examples: |
|
Seriously though, I think we should have both unopinionated (inherit styles mostly from the theme) and opinionated patterns. So I think it's okay to have some dark header/footer patterns which require more opinionated styling. I'm also interested in VIv's feedback here. |
|
👍 The main/light patterns should take style direction from the theme. I don't know if there's a way to determine that for dark versions, but I think we can define the color there otherwise. |
That way we make sure they will be visible no matter which theme is active.
albarin
force-pushed
the
fix/dark-patterns-text-color
branch
from
9818167 to
9169dd9
Compare
|
Thanks all for your feedback. |
There was a problem hiding this comment.
Thanks for updating this PR, @albarin!
I've made the light patterns inherit all the styles possible from the theme. Then for dark patterns, I've set the background to black and text/icons to white. It seems to work well for all the themes and variations I've tried.
Sounds like a good solution. I would however remove the Light word from light patterns name and just have the "regular one" and the "dark one". Ie:
- WooCommerce Large Footer
- WooCommerce Large Footer | Dark
I found a couple of small issues, mentioning them below: 👇
- Some lights patterns have hardcoded color for the social networks icons. I think we should remove it even if that means diverging from the original designs. Otherwise, there is no way to make them look good in all backgrounds:
- In WooCommmerce Essential Header | Light and WooCommerce Large Header | Light, links are still dark:
|
@Aljullu thanks for the review, just pushed fixes to the 3 issues 🙂 |
There was a problem hiding this comment.
Thanks for updating the PR, @albarin!
I could find another case where the patterns don't look good. I tried with Twenty Twenty Two modifying the colors (making the background black and the text color light):
then, the light WC patterns don't look good:
TypeScript Errors ReportFiles with errors: 448 🎉 🎉 This PR does not introduce new TS errors. |
There was a problem hiding this comment.
Style-wise it looks perfect! 👌 I noticed one last issue, however: menus seem to be different in the light and dark patterns:
I think that's only reproducible if you have created at least one menu (modifying the Navigation block). I left a comment inline where I think the source of the issue might be.
I'm pre-approving, but happy to re-review if you want. 🙂
|
👋 Looking good! I am wondering if in your screenshot @Aljullu - the site name is in black? Making the text look not quite centred? |
The site name is
|
Co-authored-by: Albert Juhé Lluveras <[email protected]>
TypeScript Errors ReportFiles with errors: 448 🎉 🎉 This PR does not introduce new TS errors. |
1 similar comment
TypeScript Errors ReportFiles with errors: 448 🎉 🎉 This PR does not introduce new TS errors. |
TypeScript Errors ReportFiles with errors: 448 🎉 🎉 This PR does not introduce new TS errors. |
albarin
changed the title
albarin
added
type: bug
…erce#7524) * Set a the color as white for the dark header/footer patterns That way we make sure they will be visible no matter which theme is active. * Adapt light patterns to inherit colors from theme * Remove light from pattern names and color from social icons * Make links inherit color * Remove background * Remove wp:page-list Co-authored-by: Albert Juhé Lluveras <[email protected]> Co-authored-by: Albert Juhé Lluveras <[email protected]>
The goal of this PR is to make the text visible on all themes on the WooCommerce header/footer patterns.
The light patterns will inherit styles from the theme and the dark ones will have black background and white text.
Fixes woocommerce/woocommerce#35248
Testing
User Facing Testing
WooCommerce Visibility
Changelog