Remove extra padding for cart sidebar #5430
Conversation
rubikuserbot
requested review from
a team and
ralucaStan
and removed request for
a team
|
Copying comments from previous thread @senadir Took me a bit of hunting, but I limited that change to this PR #4974 so we need to make sure it won't regress it as well. @senadir I tested this and failed to see what's trying to fix. A before and after screenshot would help. @alexflorisca Hi @senadir, thought I'd reply here for posterity and with some more useful details following our slack convo. Feel free to ping me on slack in that thread in response! Screenshots of the bug can be seen here. This happens mainly when the cart page has a sidebar. So it looks like when the cart block sidebar is a certain width (236px), the browser decides there isn't enough real estate and it drops the "Apply Discount" button on the second line. The styles that control the input/button come from the Woocommerce Points And Rewards plugin so we have not control over making the button smaller for example. My initial approach was to remove the extra margin added in the PR you mentioned. I realise this causes changes to the design and may not be the best solution As an alternative, we could reduce the margin to However this seems a bit arbitrary to me and i'm aware that we are trying to fix the layout for a third party plugin at a specific width. It's difficult to test the impact this might have on other plugins that add content to the sidebar and there will likely always be a case of misalignment if we change the width of the sidebar. I guess reverting the extra I'm looking for a bit of guidance here as I'm not sure what the best approach is. |
opr
added
focus: blocks
|
Size Change: 0 B Total Size: 819 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
The visual bug from #5152 does not happen anymore on trunk, so I don't think this fix is necessary. Could you confirm?
|
@ralucaStan This is still an issue for me when I run locally with latest version of WC and latest trunk of WC Blocks, using the Storefront theme. @senadir any chance you could take a look at the comment above and see what you think of the solutions I proposed? |
|
I can look into this |
| @@ -238,13 +238,6 @@ table.wc-block-cart-items { | |||
| } | |||
| } | |||
|
|
|||
| .wc-block-cart__sidebar { | |||
| & > div:not(.wc-block-components-totals-wrapper) { | |||
There was a problem hiding this comment.
In the Cart block, there is no direct child with class .wc-block-components-totals-wrapper in .wc-block-cart__sidebar. But there is in the Checkout block. So I think this is a leftover code somehow.
There was a problem hiding this comment.
I can see .wc-block-components-totals-wrapper in both checkout and cart. This solution isn't bullet proof though because at a certain width, the browser will decide to wrap the input element and the button. The ideal solution would be to set flex-wrap: no-wrap on the .wc-points-rewards-redeem-form element but that needs to be done in the points and rewards app.
There was a problem hiding this comment.
I can see .wc-block-components-totals-wrapper in both checkout and cart.
Yes, that is correct, but in the Cart is never a direct child of wc-block-cart__sidebar, like the selector here targets >. And it is a direct child in Checkout. That's why I think this could have been a common style at one point.
Exactly, setting flex-wrap: nowrap on the .wc-points-rewards-redeem-form fixes the issue if they always want to have them on the same line. With the current code, flex-wrap: wrap they actually say, hey go on another line if there is not enough space. But they don't have any styling in place for this case, like vertically spacing the button and so on.
We can actually open a ticket on their side.
There was a problem hiding this comment.
I will approve this PR:
- I think this class is a leftover, I left a comment on the code
- the issue with the form wrapping is actually in the plugin, because the form has flex-wrap: wrap but no styling for when the form wraps, goes on two lines.
- I think we need to fix this on our side tho' because the form looks good in Checkout but not in Cart and we need to be consistent. With this change we can also notice that the spacings between the two columns in C & C are now equal.
Ready to 🚢 as far as I'm concerned.
👏 Alex and Thomas
|
Thanks for your review @ralucaStan , see my inline comment, but essentially this solution as is, only solves half the problem. it works for Checkout and Cart at most screen widths, but if you play around with the browser width, resize it to around 797px wide (on the checkout page), you will see that the button also jumps down on the second line. This is a bit of an edge case, as it only happens when the screen size is between 780px - 800px. Something similar happens for the cart. Without this fix, this margin would be bigger and there would be more chance of displaying the broken layout to users. So it's not ideal, but it's an improvement. |
I think we can live with this edge case, considering the extension does not optimize the form for edge cases and it's a bug on their side. We can merge this and open a ticket on their repo like I suggest in my comment |
|
Sounds good, I'll merge it in and open a ticket with them 👍 |
Original PR #5247 by @alexflorisca - I accidentally merged and closed that PR.
When upgrading the cart, there was an extra left and right margin added to the Cart page sidebar. I'm not sure if this was intentional, however it causes a visual regression bug with the points and rewards plugin where the input box and the button don't line up properly (see issue for screenshots).
This fixes #5152
Testing
How to test the changes in this Pull Request:
Changelog