-
Notifications
You must be signed in to change notification settings - Fork 219
fix/8946: replace ToggleGroupControl with ToggleControl #9098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @Sidsector9! That's definitely a nice improvement to keep the UI consistent and more comfortable to use. 👏 I tested it and it works as expected, code changes look good as well.
The only thing missing before being able to merge it is to add testing steps, that way we have a consistent way to test this PR in releases and in the case there were regressions in the future.
@Sidsector9 is that something you can do? Thanks in advance!
@Aljullu I have added the testing instructions. |
QA/Test Report- ✅Testing Environment -
Steps to Test- For Non-Block theme (Storefront) -
For Block theme (2023) -
Test Results - The WooCommerce Mini Cart block was tested on a non-block based theme (Storefront) as well as in a block-based theme (WP 2023). The behavior of the block was consistent between the two themes, and it worked as expected. When "Open cart in drawer" was enabled, the drawer opened up to show the contents of the cart, and when it was disabled, the drawer didn't open up. Also tested the new UI of block setting in RTL supportive theme. Before Fix- After Fix- Functional Demo / Screencast - Functional.Demo.for.Setting.in.FE.mp4QA Status- Pass ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the changes as well and it looks good!
The only missing part is a linter check is failing in the file assets/js/blocks/mini-cart/edit.tsx
:
75:23 error Replace `⏎↹↹↹↹↹↹↹↹↹` with `·` prettier/prettier
77:1 error Replace `↹↹↹↹↹↹↹↹↹↹addToCartBehaviour:·value·?·'open_drawer'·:·'none'` with `↹↹↹↹↹↹↹↹↹addToCartBehaviour:·value⏎↹↹↹↹↹↹↹↹↹↹?·'open_drawer'` prettier/prettier
78:10 error Replace `}` with `↹:·'none',` prettier/prettier
79:9 error Insert `}·`
Would you be able to update it? 🙌
@kmanijak for some odd reason the If possible can someone take over this? Or I can try it again tomorrow. |
Sure, let me update the formatting and push the change to your PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's testing well, the code looks good and the pipeline passes 💪
Thanks @Sidsector9 for your contribution!
Just a heads up - As this PR changes the UI, screenshot should be updated if this block is documented anywhere. |
Fixes #8946
Accessibility
prefers-reduced-motion
Other Checks
Screenshots
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog