Mini Cart: stop using Modal component #9345
Conversation
Aljullu
added
status: needs review
type: bug
woocommercebot
requested review from
a team and
thealexandrelara
and removed request for
a team
| array( | ||
| 'selector' => '.wc-block-mini-cart__drawer .components-modal__header', | ||
| 'properties' => array( | ||
| array( | ||
| 'property' => 'color', | ||
| 'value' => $text_color ? $text_color['value'] : false, | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
I think these styles weren't used anywhere, as we weren't setting a modal header.
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
| @include visually-hidden(); | ||
| } | ||
| } | ||
| .admin-bar .wc-block-components-drawer__content { |
There was a problem hiding this comment.
This has been moved from here:
woocommerce-blocks/assets/js/blocks/mini-cart/style.scss
Lines 228 to 238 in f1714a7
|
Size Change: -27.3 kB (-2%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
✅ I have no complaints about the code
✅ I can open and close the Mini Cart drawer when the cart is empty
✅ I can open and close the Mini Cart drawer when the cart has products
✅ I can customize the styles for the Mini Cart (adding custom backgrounds, border, and/or changing the width)
✅ I can add multiple Mini Cart drawers and they work together
⛔ When you decrease an specific item quantity from 2 to 1 (it looks like it only happens between these quantities) by clicking on the decrease button for this item, the drawer does not close when you click outside its area. The only way I managed to close it (besides using the Close button) was to give a click inside the drawer area followed by a click outside it as in this video:
Screen.Recording.2023-05-10.at.17.58.31.mov
Aljullu
force-pushed
the
fix/mini-cart-drawer-component
branch
from
b3f8637 to
988b797
Compare
Good point. In bf1f53c I added some styles to reset the default Could you test it and see if it looks good to you? 🙏
Good catch! That was a super confusing one, and I still don't understand what's going on. When the In the end, I decided for not setting the |
Aljullu
force-pushed
the
fix/mini-cart-drawer-component
branch
from
988b797 to
351a216
Compare
There was a problem hiding this comment.
Great job, thank you for addressing the issues, now I can confirm that it is working well. I imagine how difficult it was to debug and find a solution for this, I'm glad that you find a way to solve it 🙌
I'm approving this PR since it is working as expected, but maybe a suggestion that I don't know if you tried is to keep a reference to the modal and when clicking on the QuantitySelector, you set the focus back to the modal, something like this:
// on Mini Cart
const modalRef = React.useRef();
<Drawer ref={modalRef} />
// on QuantitySelector if we find a way to send the modal reference or maybe to call a callback inside the onClick for the decrease button
<button
...
onClick={ () => {
...
modalRef.current.focus(); // or a callback that calls the focus() method
}}
/>
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
…the Mini-Cart drawer
… inside the Mini-Cart drawer" This reverts commit 4360f77.
…utton become disabled
Aljullu
force-pushed
the
fix/mini-cart-drawer-component
branch
from
351a216 to
4d75aa3
Compare
Oh, that's a good idea! I reverted my previous work-around in 2274917 and implemented your suggestion in 4d75aa3. I changed it a bit so instead of moving the focus to the drawer, we move it to the text input field. This keeps the code cleaner and from an accessibility point of view I think it's better because it moves the focus closer to the previous position. Thanks so much for the suggestion, @thealexandrelara! Would you mind taking another look? |
|
Thank you for implementing the suggestion 🙌 . I was testing it again, and it looks like the issue is still happening on Chrome (I'm using version 113.0.5672.92) Screen.Recording.2023-05-12.at.16.21.23.movI tested on Firefox and Safari and the drawer closes when clicking outside it 🤔 |
|
Thanks for the review, @thealexandrelara, and good catch with that issue. Honestly, I don't know why that's happening, for some reason, Chrome is moving the focus to I solved it by adding a check to see whether the focus is in the |
This PR removes the usage of the
Modalcomponent from@wordpress/components. Instead, it updates ourDrawercomponent to incorporate some functionality fromModal. I didn't copy & paste everything from theModalcomponent, instead, I just tried to import the features that were needed for our use case.Fixes #8606.
Part of #8452.
Accessibility
prefers-reduced-motionTesting
User Facing Testing
This PR shouldn't introduce any visual changes for users, so testing it means making sure there are no regressions.
Now, let's test that things keep working if there are two Mini Cart blocks in the same page. We don't officially support it, but at the same time we don't want the experience to be broken.
WooCommerce Visibility
Changelog