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 hardcoded admin colors, use admin theme colors #39182
Conversation
Test Results SummaryCommit SHA: 155420f
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. |
Hi @jorgeatorres, @rrennick, @xristos3490, @jimjasson, @jarekmorawski, 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: |
@PanosSynetos We need better testing instructions for this. I installed this and edited the plugin header version to 7.8.0. I was able to update from the plugins page without getting the popup.
This will need instructions on blocking an api call as I wasn't able to get the empty screen on any |
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.
@PanosSynetos Nice work on this. All of the things that I was able to test looked great.
I edited the testing instruction in some places to add details for the QA folks that will test this. Plus I left the comment regarding a couple screens I couldn't trigger/access. I tested this on an AT site so I didn't test any calypso bridge specific changes. It would be good to clarify when testing needs a wccom site with calypso.
I forgot to mention that https://github.com/woocommerce/woocommerce/files/11806335/test-plugins-37343.zip contains a |
Thank you @rrennick for your feedback and the changes you've made.
I'll update the instructions during the day and let you know |
Hey @rrennick - I've updated the missing instructions - hope we're good now. Thank you for your suggestions, you were spot on! |
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.
Tested this and looks good! ✅
I used the sunrise theme in my tests.
Empty states and database upgrade
Importer
Toggles
Status Icons
Breadcrumb up icon
Preview
Update Core
Shipping empty states
Auth screen
WC Admin Empty Content
Marketplace
Marketplace Helper
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.
LGTM. Great job on this one @PanosSynetos. Thank you! 🙇
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.
Looks good! Great job showcasing each change! 👌
💡 The task has finished, and I've asked for a CFT pdqkbG-2V7-p2 - If there are any comments, please add them in the CFT comments
Submission Review Guidelines:
Changes proposed in this Pull Request:
With this PR, I use the admin theme colors wherever possible, so we harmonize the experience.
Wherever it was not possible to use the variable, I've either used grey or the new woo pink.
Closes #37172, Closes #37166, Closes #30829, Closes #29267, Closes Automattic/wc-calypso-bridge#1215 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
wp-admin/update-core.php
modal)Default
underUsers -> Profile
The changes can be tested on any site, Atomic, WPCOM, Self Hosted, Woo Express, WoA dev - it doesn't need Calypso; no changes have been done on Calypso.
Here are the before/after screenshots so you can validate the changes.
Empty states
All empty state buttons are affected by this change. I decided to remove the hardcoded colors, and let the admin theme color.css do its magic.
There is no need to test every single empty state above. The CSS selectors are common, so if you test one of the above, consider the rest tested.
Reminder: the empty state is not shown if the trash has items.
Importer
Toggles
All toggles throughout the admin are affected by thing change. Status icons are updated too.
🔔 The pink question-mark comes from WC Payments. I’ll open a PR in WC Pay to fix this color, once this PR gets merged.
Status Icons
Breadcrumb up icon
WooCommerce notices
React Notices
Thank you @xristos3490
Hover on order preview
Update Core
That's a tricky one to trigger; see
for replication steps
Alternative/easier method, you can download a zip from pdqkbG-2V7-p2 , install it, visit
wp-admin/update-core.php
, open the browser console and executejQuery( '#wc-upgrade-warning' ).trigger( 'click' )
Changes
Shipping empty states
This screen has been slightly modified, apart from the button colors
Auth screen
That was a catch from @beaulebens
WC Admin Empty Content
To trigger the empty content,
Another one from @beaulebens
I couldn’t change the icon to have the admin theme color, as it needed to modify a react component (which others extend), so I took the liberty to make it grey to avoid having to change it in the future.
Marketplace
Marketplace Helper
Known issues
The following two issues are not related to WooCommerce Core, but since you might see them in the settings, I thought I should mention them, to avoid unnecessary reports.
WooCommerce Payments
The icon needs to change on the WC Payments side.
WooCommerce Services
The colors need to change on the https://github.com/Automattic/woocommerce-services repo.
Changelog entry
Significance
Type
Message
Comment