Refactor: Use an alternative mini cart icon to fix the Mini Cart styling issues #5985
Conversation
…quantity badge also doesn't need color class and style
rubikuserbot
requested review from
a team and
tjcafferkey
and removed request for
a team
dinhtungdu
added
type: refactor
|
Size Change: +327 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
|
Requested additional reviews from Albert and Luigi because this changed how we style the Mini Cart block. |
There was a problem hiding this comment.
I like this approach and it's really cool to see how much code could be removed. 👏 My only concern is that it removes the background color setting. Was that intentional? I think we should try to keep it.
Ie, in trunk it's possible to change the styles of the Mini Cart button to something like this:
|
@Aljullu I added the background back in 5a11609
Yes, I removed the background because I don't want to use experimental features and the background setting was initially created for the badge background. But I agree with you that having the ability to set the background for the button is actually useful. |
There was a problem hiding this comment.
Works perfectly now, thanks @dinhtungdu! I just have one last question: why do we need to use __experimentalSelector and __experimentalSkipSerialization? Can't we apply the background styles directly into the wrapper element (.wc-block-mini-cart)?
@Aljullu If we apply the style to the wrapper element, this will happen.
At glance, we can solve this problem by styling the wrapper as an inline block. But we will have an issue with alignment because we need the wrapper full width (to its container) for the alignment to work. By default, the Global style target the wrapper element ( About Alternative optionsActually, there is another way to solve this problem, to make styling work without experimental features
We can apply the approach of the |
There was a problem hiding this comment.
Oh, I missed that, thanks @dinhtungdu! I was in fact testing it inside a Row block, that's why I missed it. 😅
Let's merge this PR as-is. However, I would like us to try to get rid of those experimental APIs. Otherwise, we can only enable background color controls in the Feature Plugin (but not in WC core), and that would be a missed opportunity. 😞
We can apply the approach of the block to Mini Cart block by removing the alignment and styling it as an inline element. If we need alignment, we can use a block and add as an inner block. By doing so, we don't need experimental flags and we can reduce even more lines of code. But it makes the block harder to use at the same time IMO.
I like this solution. How do you feel about creating a PR to experiment with it? Honestly, I think most usages of the Mini Cart block will come once we start adding it to patterns, so we will be able to control where it renders. Another thing to consider is that at some point we would like to make the Mini Cart block available inside the Navigation block, that would remove another instance of needing it to have alignment for itself.
cc'ing @gigitux here too for his thoughts on this.
I agree with @dinhtungdu's thoughts. I guess, in accordance with our goal, for the next block we should follow the alternative solution. We should avoid using custom style and use the blocks that Gutenberg makes available for positioning as much as possible. |
dinhtungdu
changed the title
#6002 is the attempt to remove the experimental flags and simplify the codebase. |
Fixes #5930
The current Mini Cart icon leads to some difficulties and style issues:
__experimentalSkipSerializationto target inner elements for global style. We also need to calculate manually color classes and style inside the block component.This PR tries to solve all of the above issues by using the icon as proposed here. By using that icon, we can:
__experimentalSkipSerialization, let Blocks Editor and Site Editor target the Mini Cart block using.wp-block-woocommerce-mini-cart.Accessibility
prefers-reduced-motionOther Checks
Screenshots
Testing
Automated Tests
Manual Testing
How to test the changes in this Pull Request:
User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
Notes