-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 UIColor primary #23621
Fix UIColor primary #23621
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha 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.
Approved, and I snuck in a small fix for widgets
static let secondaryLineColor: UIColor = UIColor(light: .quaternaryLabel, dark: .tertiaryLabel) | ||
static let primaryLineColorViews: UIColor = UIAppColor.blue(.shade50) | ||
static let primaryLineColorVisitors: UIColor = UIAppColor.purple(.shade50) |
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.
The charts like "Insights" and "Subscribers" are designed to use these colors in both wpios and jpios to match wp.com. It is a current production behavior based on the customer feedback. The green color is not great for charts because it implies "growth" and can be confusing.
There seem to be an issue with the empty charts:
Let's make sure the changes match the current production behavior.
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.
This should be resolved now, merging.
5253db3
to
cd8a5a4
Compare
RCA
In the previous version,
UIColor.primary
(custom property) meant the same asUIColor.brand
(blue/green).In the new version,
UIAppColor.primary
maps toUIColor.label
, which is incorrect. In most places whereUIColor.primary
was replaced withUIAppColor.primary
, the app now shows incorrect colors. For example, here are three areas I found during testing:The fix isn't as straightforward because in some places – the ones I found so far –
UIAppColor.primary
replaced other colors likeUIColor.appBarText
. We don't want nav bar titles to be green or blue, so it had to be address it with more code changes.To test:
I've reviewed a bunch of scenarios that used
UIAppColor.primary
, but didn't have to time to check everything. I hope if there are any other issues, they will eventually bubble up. To the very least, this PR should reduce the number of issues.Future Changes
There is still a little bit of work left to review how the brand/tint/primary colors are used and how the colors are applied in the UI.
For Jetpack, the rule of thumb should be to use green color (
UIAppColor.brand
) only in places where it would otherwise be hard or impossible to tell that the control is a control. The main tint color for the Jetpack app (UIAppColor.tint
) is black (UIColor.label
).I would also recommend removing
UIAppColor.primary
as it can be confused withColor.primary
from SwiftUI.Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: