Product Button: Remove the hardcoded width and padding #11537
Conversation
…gin for :before and :after
|
@jarekmorawski I'd appreciate your input regarding the proposed solution in this PR to the button width problem. |
woocommercebot
requested review from
a team and
thealexandrelara
and removed request for
a team
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: -71 B (0%) Total Size: 1.54 MB
ℹ️ View Unchanged
|
|
Looks good to me! Nice work, @danieldudzic. 👏
It's a good compromise and the difference is night and day when it comes to overall order and harmony. |
There was a problem hiding this comment.
Confirming it solves the problem, I really like that we are trying to make this kind of visual improvements. 👏 Code-wise the PR looks good to me. The only concern I have is that now it's quite easy for words to break into two lines (screenshots from TT3):
(See how the word products in the second button is broken even though there is enough horizontal space for it)
After thinking about this, I think there are two approaches that we could investigate:
- It looks like these buttons have
word-break: break-word. I'm not sure why is that, but maybe it's safe to remove it? I did a quick test and IMO it looks better:
- Given that with this PR we are adding margin to the
:beforeand:after, I wonder if it would make sense to remove the default horizontal padding from the button. I mean, specifically setting it to 0 so the theme padding doesn't add on top of the:beforeand:aftermargins. Do you think that would work? (If we go that route, I'm not sure if it would be better to do it via global styles or directly in CSS)
| Before | After |
|---|---|
 |
 |
|
@Aljullu thanks for testing!
|
…ocks into fix/11436-button-width
…ce/woocommerce-blocks into fix/11436-button-width
|
Thanks for checking and good catch with those issues, @danieldudzic!
A quick fix could be to move the spinner a little bit to the left. I think that way it might look "aligned" without needing to have double horizontal padding. However, the more I think about this, the more I think we shouldn't work-around the "jumping width" issue. The reason I'm mentioning that is because:
So my proposal would be to remove the hard-coded 150px width as you did, but not try to fix the "jumping width" issue (because of the issues mentioned above). I know that's not ideal. So in the future, we could consider changing this spinner effect (which causes the "jumping width" issue) for something else: either simply disabling the button while "adding to cart" or doing something like the Proceed to Checkout button of the Cart block, which allows keeping the button the same size: Enregistrament.de.pantalla.des.de.2023-11-03.10-39-32.webmOf course, that would need design input and it's out of the scope of this PR. But I feel that would be the more future-proof solution, as the current Add to Cart spinner will always either cause the "jumping width" issue or need some extra padding which might be confusing with global styles. What do you think? |
This would also require some smart adjusting of padding/margins so that the width of the button doesn't jump. Potentially should be possible.
I normally would agree with you, but here we have a situation where the button currently doesn't jump because of the static width. So that change will alter the existing behavior and the buttons will start jumping.
Yes, I believe a design exploration into this would be useful. I like the spinner replacing the text altogether. I think the proposed margin solution in this PR could be an interim fix to the overflow issue. But I don't feel strongly one way or another :) |
|
Moving to 11.6.0 |
Yeah... I see what you mean, but at the same time, any CSS change that we make will alter the existing behavior. In However, given that we need to alter the existing behavior no matter what, I think we should move towards the solution that we feel will be more future-proof and cause less issues with existing themes. I'm advocating for removing the hard-coded width while not adding margin to
Let me know what you think! Also, happy to jump in a sync-combo if you prefer. 🙂 I acknowledge my proposal is not perfect, but I consider the pros of not adding margins to |
|
@Aljullu All valid points. Thank you for championing a better Add to Cart button experience 🙏 I'm happy to follow your proposed direction on this. Even with the drawbacks, the "jumping-width" button solution seems to be a bit more robust. |
There was a problem hiding this comment.
I tested on TT1, TT2, TT3, TT4 and Storefront and it looks great on all of them! Thanks for the update, @danieldudzic! 🚢
What
This PR removes the hardcoded button width and padding (to inherit the padding from the theme styling).
Fixes #11436
Why
Currently, the width of the buttons is hardcoded to
150px. My assumption is that this was introduced to prevent the following effect of "jumping width" because of the spinner:The hardcoded width is less than ideal as it leads to issues like #11436.
Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
4-Column Product Rowpattern to a post or a page.Shoppage and make sure the buttons look and behave correctly.Screenshots or screencast
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog