-
Notifications
You must be signed in to change notification settings - Fork 114
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
[Brand Updates] POS theme updates #14756
base: feature/woo-2.0-brand-updates
Are you sure you want to change the base?
[Brand Updates] POS theme updates #14756
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
) | ||
) | ||
} | ||
static var posPrimaryButtonBackground: Color = .accent |
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.
For the regular app theme, the primary button uses purple_40
in all themes, this is to give a better contrast with the white text (more context here p1734705352565459/1727138732.960149-slack-C07E7D0R761), but for POS, I noticed the button text is black, so for now, it will use purple_30
in dark theme.
Was the usage of black text here a design decision or just a dev choice? Personally, I think white looks better and gives a better contrast, but that's just a thought.
Black | White |
---|---|
) | ||
) | ||
} | ||
static var posSecondaryButtonForeground: Color = .accent |
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 POSSecondaryButtonStyle
was referencing posPrimaryButtonBackground
. When I first changed posPrimaryButtonBackground
to use a single color for both themes, I added this property. However, I later modified the primary button background again (see the comment above), so this property isn't strictly necessary now. Still, in my opinion, retaining it could enhance clarity and help decouple the components. Let me know your thoughts.
@@ -17,7 +17,7 @@ struct POSProgressViewStyle: ProgressViewStyle { | |||
lineWidth: lineWidth, | |||
lineCap: .butt, | |||
circleColor: Color(.wooCommercePurple(.shade10)), | |||
fillColor: Color(.wooCommercePurple(.shade50)) | |||
fillColor: Color.accent |
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 updated this to match the updated app accent, which is purple_40
in light theme and purple_30
in dark theme.
This matches the color we use in the last Figma playground files (rNAclWFAdQRw7EXEcsNVai-fi-2533_5542) where we are are using purple_40
on light theme.
Thanks, @hichamboushaba for the changes! We'll make sure we're aligned with the decisions before merging. Feel free to leave it for us to finish & merge. |
Part of: woocommerce/woomobile-private#382
Description
This PR updates the POS theme colors to match the new brand, it introduces a new
accent
color that usespurple_40
in light theme andpurple_30
in dark theme, and updates the different components to use it.Note: If we are worried this update might break something, I'm fine with dropping it of the project.
Steps to reproduce
Testing information
Tested using iPad Air simulator, iOS 18.1
Screenshots
RELEASE-NOTES.txt
if necessary.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: