Add global style support to Mini Cart (button) #5100
Conversation
dinhtungdu
added
type: enhancement
rubikuserbot
requested review from
a team and
tjcafferkey
and removed request for
a team
|
Size Change: +565 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
dinhtungdu
added
the
skip-changelog
| @@ -31,6 +31,8 @@ const renderMiniCartFrontend = () => { | |||
| getProps: ( el: HTMLElement ) => ( { | |||
| isDataOutdated: el.dataset.isDataOutdated, | |||
| isInitiallyOpen: el.dataset.isInitiallyOpen === 'true', | |||
| backgroundColor: el.dataset.backgroundColor, | |||
There was a problem hiding this comment.
You will need to define textColor here too, right?
There was a problem hiding this comment.
Taking another look at this... I wonder if we could read the class names from the elements instead of passing the colors in data attributes. I'm thinking something along these lines:
quantityBadgeClassnames: el.querySelector('.wc-block-mini-cart__quantity-badge').classListThen, we would just pass that array to the React component so it can apply those classes. This way, we wouldn't need to call getColorClassName() from the JS code, which allows us to avoid one dependency: @wordpress/block-editor. 🙂
There was a problem hiding this comment.
Thanks so much, it's a great idea!
There was a problem hiding this comment.
I update the related components to follow this approach.
| .editor-styles-wrapper .wp-block-woocommerce-mini-cart.wc-block-mini-cart { | ||
| background-color: transparent !important; | ||
| } |
There was a problem hiding this comment.
Oh, I'm sorry, this is from the previous implementation. Thanks for pointing this out!
There was a problem hiding this comment.
Update: We still need it. The preset style classes in the editor come with !important that override our button style.
There was a problem hiding this comment.
Ah, right, I could reproduce the issue. However, I think that might be a sign that there is something else that we are not doing correctly here. I took a look at the Gutenberg Table block as an example: it also has to apply the background to an inner element (<table>) instead to the wrapper (<figure>); but they don't have this issue.
After some investigation, I think the difference is that they use __experimentalSkipSerialization. Do you think we could use it too?
There was a problem hiding this comment.
@Aljullu I added __experimentalSkipSerialization in 27a2584 (#5100)
| __experimentalSelector: | ||
| '.wc-block-mini-cart__button, .wc-block-mini-cart__badge', |
There was a problem hiding this comment.
Everything seemed to work fine for me when I removed these lines. Why was it needed? 🤔
There was a problem hiding this comment.
If you remove this, the global style doesn't work correctly. Without this, Gutenberg will add global style to .wp-block-woocommerce-mini-cart instead of the above selectors. This is fine for text color (by inheriting the parent element) but doesn't work for background color.
There was a problem hiding this comment.
Is the GB team aware that we're using this experimental supports property and why we need to use it? If not, it might be handy to make sure they know somehow so there's awareness of it's usefulness for other projects when it comes time to evaluate graduating it.
There was a problem hiding this comment.
@nerrad I'm not sure if they know about it or not. I will add comments explaining why we need them.
Edit: added comments for __experimentalSelector and __experimentalSkipSerialization.
There was a problem hiding this comment.
Given the discussion in p1637141712378200-slack-C02FL3X7KR6, I think we can go ahead using those __experimental features for now, given that the Mini Cart block is gated to the Feature Plugin.
In the future, while we research global styles in a bigger scale for all blocks, we can get a list of all experimental features we need to use and get in contact with Gutenberg devs to know their plans with them.
There was a problem hiding this comment.
Thanks for updating the PR @dinhtungdu. This is working great, I just left some more comments below, but overall this should be ready to merge. I'm not approving yet because I would like to double-test it once more after the changes are done.
| .editor-styles-wrapper .wp-block-woocommerce-mini-cart.wc-block-mini-cart { | ||
| background-color: transparent !important; | ||
| } |
There was a problem hiding this comment.
Ah, right, I could reproduce the issue. However, I think that might be a sign that there is something else that we are not doing correctly here. I took a look at the Gutenberg Table block as an example: it also has to apply the background to an inner element (<table>) instead to the wrapper (<figure>); but they don't have this issue.
After some investigation, I think the difference is that they use __experimentalSkipSerialization. Do you think we could use it too?
Replace `getColorClassName` and manual style manipulation...Replace `getColorClassName` and manual style manipulation with `useColorProps` once the hook is no longer experimental. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/cd853d4f080a42799f272e72c48d514a3d7d01e0/assets/js/blocks/cart-checkout/mini-cart/edit.tsx#L46-L58🚀 This comment was generated by the automations bot based on a
|
Replace `getColorClassName` and manual style manipulation...Replace `getColorClassName` and manual style manipulation with `useColorProps` once the hook is no longer experimental. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/27a2584e1ba0b2a88581cd13453b65ee12694dc2/assets/js/blocks/cart-checkout/mini-cart/edit.tsx#L46-L58🚀 This comment was generated by the automations bot based on a
|
There was a problem hiding this comment.
Good job in this PR @dinhtungdu! Thanks for investigating and dealing with all the challenges of adding global styles to a block such as the Mini Cart.
I'm pre-approving but left some comments below.
| __experimentalSelector: | ||
| '.wc-block-mini-cart__button, .wc-block-mini-cart__badge', |
There was a problem hiding this comment.
Given the discussion in p1637141712378200-slack-C02FL3X7KR6, I think we can go ahead using those __experimental features for now, given that the Mini Cart block is gated to the Feature Plugin.
In the future, while we research global styles in a bigger scale for all blocks, we can get a list of all experimental features we need to use and get in contact with Gutenberg devs to know their plans with them.
Replace `getColorClassName` and manual style manipulation...Replace `getColorClassName` and manual style manipulation with `useColorProps` once the hook is no longer experimental. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/c323f214b1ca5800ad8c43abd914b35dc9050da6/assets/js/blocks/cart-checkout/mini-cart/edit.tsx#L46-L58🚀 This comment was generated by the automations bot based on a
|
refactor the logic of color class and style using StyleAt...refactor the logic of color class and style using StyleAttributesUtils. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/c323f214b1ca5800ad8c43abd914b35dc9050da6/src/BlockTypes/MiniCart.php#L238-L249🚀 This comment was generated by the automations bot based on a
|
src/BlockTypes/MiniCart.php
Outdated
| @@ -222,6 +224,39 @@ protected function get_markup() { | |||
| $cart_contents_total += $cart->get_subtotal_tax(); | |||
| } | |||
|
|
|||
| $wrapper_classes = 'wc-block-mini-cart wp-block-woocommerce-mini-cart'; | |||
There was a problem hiding this comment.
Because we use __experimentalSelector, we don't need wp-block-woocommerce-mini-cart here anymore.
Replace `getColorClassName` and manual style manipulation...Replace `getColorClassName` and manual style manipulation with `useColorProps` once the hook is no longer experimental. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/a6c5b348a8ce1b62f69bae2f45d659c564122099/assets/js/blocks/cart-checkout/mini-cart/edit.tsx#L46-L58🚀 This comment was generated by the automations bot based on a
|
Replace `getColorClassName` and manual style manipulation...Replace `getColorClassName` and manual style manipulation with `useColorProps` once the hook is no longer experimental. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/04cca3c28e33903d000375bf3b334cba41aae46f/assets/js/blocks/cart-checkout/mini-cart/edit.tsx#L46-L58🚀 This comment was generated by the automations bot based on a
|
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
This is a part of #4965.
This PR:
true).Notes: The current limitation of this PR is global style doesn't work for background.
Updates: The global background support was added using
__experimentalSelector. It's possible to not use experimental features but in my discovery, it only works for the PHP side. See this.Screenshots
Screen.Recording.2021-11-09.at.13.05.53.mov
Testing
How to test the changes in this Pull Request:
Follow the screencast above or:
User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).