-
Notifications
You must be signed in to change notification settings - Fork 137
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
Update reader illustrations #13204
Update reader illustrations #13204
Conversation
29a42a7
to
2e05d2a
Compare
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
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.
Android Lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/woo-2.0-brand-updates #13204 +/- ##
================================================================
Coverage 40.59% 40.59%
Complexity 6372 6372
================================================================
Files 1345 1345
Lines 77236 77235 -1
Branches 10602 10602
================================================================
Hits 31352 31352
+ Misses 43136 43135 -1
Partials 2748 2748 ☔ View full report in Codecov by Sentry. |
2e05d2a
to
56f1aed
Compare
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 and I don't see any issues with the new illustrations. I tested the screens using the site from the smoke testing guide, in Dark and Light mode on a Pixel 4 Simulator.
I noticed some readability issue on bigger font sizes but I don't think it's related to this PR, even though I see you fixed a layout issue here also.
I've reported that to peaMlT-14Y-p2
Hi there! Thanks for requesting to review the screens. They look great! There's only one screen that needs an update. The Here's the link to the screen in Figma. The reasons behind the changes are:
If it's not a considerable amount of work 😅, is it possible to update the white outlined reader to purple if it's connected? Thanks! |
Thanks for the review, @wagveloso! I'll update the illustration and indicator color on the linked screen. I’ll also change the selected indicator color to purple, as we did on the login carousel.
If you mean this white-outlined reader, |
Closes: woocommerce/woomobile-private#378
Description
This updates the illustrations appearing in the reader connection and payment flow.
Additionally, a layout issue has been fixed (f001f14). You can see the issue in the "before" column of the third row.
Steps to reproduce
Apply this patch to show a single reader in the reader connection flow: Always_show_single_reader.patch
Testing information
Please test in both light and dark modes.
The tests that have been performed
Images/gif
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: