Featured Items: Remove inline style defaults for color options #7036
Conversation
danielwrobert
added
needs feedback
focus: global styles
|
The release ZIP for this PR is accessible via: |
|
Size Change: -35 B (0%) Total Size: 875 kB
ℹ️ View Unchanged
|
danielwrobert
changed the title
danielwrobert
added
block: featured category
There was a problem hiding this comment.
Thanks for your research in this tricky issue, @danielwrobert! I took an initial look at this PR, but it seems like the global style changes (made in the site editor) aren't reflected in the frontend. Can you reproduce as well?
| Editor | Frontend |
|---|---|
 |
 |
|
@Aljullu no I can not reproduce what you're seeing. I'm getting the expected result:
Do you happen to have the Gutenberg feature plugin active? That is a separate issue where the latest version seems to prevent Woo and Jetpack blocks from properly rendering Global Styles to the frontend. |
danielwrobert
force-pushed
the
update/feat-item-color-defaults
branch
2 times, most recently
from
4a03834 to
1ff3c6b
Compare
rubikuserbot
requested review from
a team and
Aljullu
and removed request for
a team
There was a problem hiding this comment.
Oh, my bad, you are right. The issue was from Gutenberg (for reference, it's this one: WordPress/gutenberg#40808). Disabling GB fixes it.
Confirming this PR fixes the issue.
| .editor-styles-wrapper .wp-block-woocommerce-featured-category { | ||
| @extend %wp-block-featured-item; | ||
| } |
There was a problem hiding this comment.
If the purpose of this ruleset is only intended to fix #6489, I wonder if we should make it explicit in some way (maybe with a comment?).
Also, not blocking but wondering if instead of implementing this work-around here, we could update Storefront to avoid causing this issue. The problem seems to be that this ruleset has too much specificity and messes up the Featured Product/Category blocks, right? Maybe we could refactor that code from Storefront in some way? (I didn't really investigate that, just thinking out loud in case you have an idea, @danielwrobert 🙂 )
There was a problem hiding this comment.
If the purpose of this ruleset is only intended to fix #6489, I wonder if we should make it explicit in some way (maybe with a comment?).
@Aljullu this still really doesn't fix #6489 on Storefront's side of things. I really only added this ruleset in to add a little bit more specificity to the styles in the editor, more generally, but it should still work the same way without this.
Thinking about it more, I'll go ahead and remove this ruleset from the editor stylesheet because it's not really consistent with everything else and this styling is intended for both the frontend and admin.
Also, not blocking but wondering if instead of implementing this work-around here, we could update Storefront to avoid causing this issue.
Yes, that's 100% my thinking. IMO, the theme styles should be responsible to ensure they are not unintentionally overriding global (or other) settings. I noted this on the #7042 but should have reiterated my point here.
So next steps: I'll go ahead and remove the editor styles from this PR, as they're not really necessary, and merge. Then I'll make a note on #6489 (for posterity) and open a new issue on Storefront to address the problem you noted with the overly specific ruleset there.
There was a problem hiding this comment.
I'm a bit confused, this PR seems to reintroduce #6489. It's not clear to me if that's a known issue or not.
So next steps: I'll go ahead and remove the editor styles from this PR, as they're not really necessary, and merge. Then I'll make a note on #6489 (for posterity) and open a new issue on Storefront to address the problem you noted with the overly specific ruleset there.
It sounds good to me, but wondering if we should prioritize this fix in Storefront. Otherwise that would be a regression. 😕
There was a problem hiding this comment.
I'm a bit confused, this PR seems to reintroduce #6489. It's not clear to me if that's a known issue or not.
@Aljullu that's correct. The issue is that the way the initial fix for #6489 was handled (via inline style attributes) prevents the global styles from being applied in the editor - which can be more confusing for users, IMO.
The idea here is to revert back to the original color rules and then handle the issue in #6489 on the Storefront side. Generally, it seems better to handle theme-specific issues from the theme side, since we won't be able to account for what different themes do that may override our plugin styles.
I agree that this should be prioritized in Storefront. I am planning on getting that going today so it doesn't get put on the back-burner and the regression gets rolled out to our users.
I can even hold off on merging this until the Storefront PR is further along, if you think that'd be better?
Additionally, @sunyatasattva suggested a broader conversation on P2 around how we prioritize styles overall, which I plan to post as a follow-up as well.
There was a problem hiding this comment.
Your explanation makes sense, sorry if I misunderstood it before. Your next steps look good to me.
I can even hold off on merging this until the Storefront PR is further along, if you think that'd be better?
I will leave this up to you. Storefront has a shorter release process, so I think we are good if we merge this PR as-is and work on a fix in SF afterwards.
There was a problem hiding this comment.
Sounds good, thanks for the follow-up! I've created an Issue on Storefront and assigned myself to address ASAP.
There was a problem hiding this comment.
Yea just to chime in here as well, I think it is unclear what is the expected hierarchy between styles, and who should be concerned of being compatibile with what. Should themes make sure they are compatible with plugins or viceversa? I think we should probably come up with a scalable solution for these cases in general. 🤔
There was a problem hiding this comment.
@sunyatasattva I agree! I'll kick off a broader P2 discussion around this today.
For this particular PR, the default color styles preventing the Global Styles from working seems a bit more confusing for a user, as opposed to allowing a theme's defaults to override and impact the color contrast of the Featured Items blocks.
I'm totally open to being convinced otherwise and I'm also not opposed to closing this PR out if it doesn't seem like a good way to go!
Overall, it's a bit of a tricky situation where we want the Global Styles to function as expected while also preventing themes from overriding the default colors with poor contrast.
I'm actually wondering if it's acceptable to move forward with removing this default and not worry about the overriding color contrast issue in Storefront? In that scenario the user still has control to adjust the text color and the background overlay, as needed.
Alternatively, we could also remove the dark background overlay default as well and leave all of the color adjustments to the user when they are using the block. I'm not certain this is a good option, just throwing it out there. I'd love to hear your thoughts!
There was a problem hiding this comment.
I'm 100% with you: definitely default color preventing Global Styles is a mistake. Not providing a sensible default… I'd be against that. How all these variables interact with each other… that's where it gets really tricky and it's definitely a philosophical question at this point :)
There was a problem hiding this comment.
I'm 100% with you: definitely default color preventing Global Styles is a mistake.
Since this seems to be the general consensus, I will go ahead and merge this PR to resolve that problem.
We can then continue to explore the best route forward for the text overriding issue with Storefront in this discussion.
danielwrobert
force-pushed
the
update/feat-item-color-defaults
branch
2 times, most recently
from
c359d42 to
800daad
Compare
This replaces the inline style with an added editor-specific styling with slightly more specificity - but not too much to where it overrides the Site Editor.
These styles are meant to apply to both the frontend and the editor. The ruleset was originally added to the editor stylesheet to add an additional layer of specificity but it was decided that this is unnecessary.
danielwrobert
force-pushed
the
update/feat-item-color-defaults
branch
from
800daad to
fd7b463
Compare
…mmerce#7036) * Remove inline style defaults for color options. This replaces the inline style with an added editor-specific styling with slightly more specificity - but not too much to where it overrides the Site Editor. * Remove additional styles from editor stylesheet. These styles are meant to apply to both the frontend and the editor. The ruleset was originally added to the editor stylesheet to add an additional layer of specificity but it was decided that this is unnecessary.
This replaces the inline style in the Featured Product and Featured Category blocks with an editor-specific styling that has a bit more specificity - but not too much to where it overrides the Site Editor styles.
Fixes #7042
Screenshots
Testing
WooCommerce Visibility
Changelog