Allow customer account block to center align #9750
Conversation
roykho
added
type: bug
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: +2.52 kB (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
Hi @thealexandrelara - just checking if you had a chance to look at this PR? Let me know if you have questions. |
There was a problem hiding this comment.
Hey @roykho, sorry I totally missed your PR. I left some suggestions here that I'd like to know your opinion about them
| &.aligncenter > a { | ||
| justify-content: center; | ||
| } |
There was a problem hiding this comment.
If you accept the solution that I posted above, we can also change this css:
| &.aligncenter > a { | |
| justify-content: center; | |
| } | |
| &.aligncenter { | |
| width: fit-content; // or max-content | |
| } |
…m/woocommerce/woocommerce-blocks into fix/customer-account-align-center
|
@thealexandrelara - I pushed up changes to use |
Yeah I am not sure either however I do know using |
|
Thanks for looping me into this discussion. 🙏 I tend to agree with the use of flexbox over floats. It's more flexible, and easier to maintain in the long run. Additionally, it provides better control over alignment and distribution of space among items in the container. One concern that comes to mind is the potential impact on themes having custom CSS for the Customer Account block. I believe we should consider the implications carefully. 🤔 CC: @tjcafferkey Furthermore, is there a particular reason why we're not combining these styles? From my perspective, these styles are shared between the editor and frontend. Hence, we can consolidate all these into a single file, namely, What do you think? |
|
I bumped the PR to the next milestone. |
The reason is because the editor has a very specific CSS declaration of the
|
|
Hi @roykho, Can't we combine the selectors like this? .editor-styles-wrapper .is-layout-constrained > .wc-block-editor-customer-account.alignright,
.is-layout-constrained > .wp-block-woocommerce-customer-account.alignright {
float: none;
justify-content: flex-end;
}
.editor-styles-wrapper .is-layout-constrained > .wc-block-editor-customer-account.alignleft,
.is-layout-constrained > .wp-block-woocommerce-customer-account.alignleft {
float: none;
justify-content: flex-start;
}
.editor-styles-wrapper .is-layout-constrained > .wc-block-editor-customer-account.aligncenter,
.is-layout-constrained > .wp-block-woocommerce-customer-account.aligncenter {
justify-content: center;
}By combining the selectors, it becomes much easier to manage and modify the styles. Instead of updating multiple separate selectors, we can make uniform changes by modifying a single combined selector. This approach improves maintenance and reduces the chances of inconsistencies. What are your thoughts on this? Am I missing something? 🙂 |
|
Hi @imanish003 - yes you can combine them however then you would have editor only classnames mixed into your frontend stylesheet. That's the reason why I didn't do that. But if you think that is ok, I can do that. |
|
@roykho I see your point about keeping the editor only classnames separate from the frontend stylesheet for cleanliness and organization. One potential solution could be to use a SASS mixin, where the shared styles are defined once and then included wherever they're needed. This way, we can maintain the separation of editor and frontend stylesheets, while still keeping the shared styles in one place. Something like this: And then importing it into What are your thoughts on this? 🙂 |
|
@imanish003 - ok I made the changes based on your recommendation. Just changed the mixin name to more closely match what it is doing. |
There was a problem hiding this comment.
Hi @roykho, I tested the PR, and it's working as expected. Well done! 🚀 I genuinely appreciate your patience with my nitpicking 🙏
|
Answering as I was pinged (sorry, I know I'm a few days late 😓 ).
I don't have a strong opinion, to be honest. I think there are pros and cons of each approach. My first inclination when reading the issue was to set a width to the wrapper (something like Having said that, I think the solution implemented in this PR makes sense as well, so 👍 from my side. In the end, I expect this block to be used inside a Group or Row block, so alignment shouldn't be something that is frequently used. |
Fixes #8412
This PR fixes the issue with the
Customer Accountblock where using the center align layout is not working.Screenshots
Testing
User Facing Testing
Customer accountblock.center. Update/Publish.WooCommerce Visibility
Performance Impact
Changelog