New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix "Marketing & Merchandising" padding on store management panel #38088
Conversation
…ng & Merchandising heading
Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: e4af20d
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
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.
Minor comment on spacing standard
@@ -3,3 +3,7 @@ | |||
.woocommerce-store-management-links__card-body.is-size-custom { | |||
padding: 24px 0 8px 0; | |||
} | |||
|
|||
.woocommerce-store-management-links__card-body { | |||
padding-top: 24px; |
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 fix, @chihsuan! However, I'm seeing the 24px
padding at the top is not symmetrical with the 16px (or $gap
) that's used for the typical vertical spacing around widget children.
padding-top: 24px; | |
padding-top: $gap; |
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.
Good eye! @ilyasfoo What do you think if we use padding: $gap-large 0 $gap-smaller 0;
(padding: 24px 0 8px 0;
)?
It looks like it was supposed to be like that.
woocommerce/plugins/woocommerce-admin/client/store-management-links/style.scss
Lines 2 to 5 in 92a1858
// We need to do this to allow item hover backgrounds to stretch the whole card body. | |
.woocommerce-store-management-links__card-body.is-size-custom { | |
padding: 24px 0 8px 0; | |
} |
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.
Just made the changes first in 30aab39!
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.
Good catch, however, I'm seeing the padding that's added gives a "border-like" artefact at the bottom when hovering the last item:
Whereas, in inbox notes items, tasklist items do not have this.
But perhaps I may be overly complicating it. I don't know if we need to call our expert @verofasulo for opinion 🙏 😄
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'm seeing the padding that's added gives a "border-like" artefact at the bottom when hovering the last item
Good catch, @ilyasfoo! I agree that it would be better to remove the padding there to avoid that effect.
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 @verofasulo @ilyasfoo! Updated as suggested in e4af20d. 🙌
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.
Awesome, thanks @chihsuan! LGTM! 🚢
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #35583.
This PR adds a 24px spacing between the Manage my store header and the Marketing & Merchandising heading
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
WooCommerce > Home