Skip to content
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

[Woo POS] [Design System] Apply straightforward color updates #15068

Open
wants to merge 19 commits into
base: feat/ds-color-palette
Choose a base branch
from

Conversation

jaclync
Copy link
Contributor

@jaclync jaclync commented Feb 6, 2025

Part of #15061

⚠️ Currently based on #15063, only review this PR if the base PR has been approved without pending changes

  • ⚠️ Merge conflicts with trunk on previews from environment object, will address these when the base PR is merged

Description

This PR applies a few straightforward color updates to replace the previous POS color usage so that a few previous POS colors can be removed. The remaining colors will be replaced when components are created/updated:

Color Scheme Updates:

  • Renamed 3 text color properties in Color+POSColorPalette.swift to replace the previous corresponding text colors:

    • posTextPrimary to posPrimaryText
    • posTextSecondary to posSecondaryText
    • posTextTertiary to posTertiaryText
  • Updated color references in CardReaderConnectionStatusView.swift:

    • Changed Color(.wooCommerceEmerald(.shade40)) to Color.posSuccess for circle icons
    • Changed Color(.wooCommercePurple(.shade60)) to .posPrimaryText for disconnected font color

Background Color Updates:

  • Updated background colors in various views to use new color constants based on design:
    • Changed Color.posPrimaryBackground to Color.posSurface in PointOfSaleCardPresentPaymentFoundMultipleReadersView.swift
    • Changed Color.posSecondaryBackground to Constants.backgroundColor in multiple views including ItemListErrorCardView.swift, ParentProductCardView.swift, SimpleProductCardView.swift, and VariationCardView.swift [1] [2] [3] [4]

Other Changes:

  • Added a new backgroundColor constant in PointOfSaleItemListCardConstants.swift to centralize background color usage
  • Updated foreground colors for text and icons to use new color constants in ItemListView.swift and ItemRowView.swift [1] [2] [3]

Steps to reproduce

The color changes are quite scattered (I could make smaller PRs for each smaller part, but I thought it's more efficient to change a bunch with reasonable diffs so that the reviewer(s) don't have to test it many times.

I'd suggest testing the PR by launching POS, adding some product/variation(s) to cart, checking out, paying with a card & cash, emailing receipt, and starting a new order. Other things to test:

  • Tap on the products banner or notice
  • Tap on the floating control at the bottom
  • Connect/disconnect reaader

Testing information

Screenshots

Simulator.Screen.Recording.-.iPad.Pro.11-inch.M4.-.2025-02-06.at.16.31.41.mp4
Simulator.Screen.Recording.-.iPad.Pro.11-inch.M4.-.2025-02-06.at.16.33.10.mp4
Screenshot 2025-02-06 at 11 46 03 AM
  • I have considered if this change warrants user-facing release notes and have added them to 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@jaclync jaclync added type: task An internally driven task. feature: POS labels Feb 6, 2025
@jaclync jaclync added this to the 21.8 milestone Feb 6, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 6, 2025

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr15068-b2529ec
Version21.6
Bundle IDcom.automattic.alpha.woocommerce
Commitb2529ec
App Center BuildWooCommerce - Prototype Builds #12826
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@jaclync jaclync marked this pull request as ready for review February 6, 2025 09:38
@jaclync
Copy link
Contributor Author

jaclync commented Feb 6, 2025

@iamgabrielma @joshheald @staskus 👋 If you think the PR should be merged to a feature branch, please let me know. I personally prefer merging to trunk so that we can catch any unexpected issues earlier with more devs working on other parts of the app and we won't end up with a giant PR. Additionally, there are 0/few production usage of POS and I think it's safe to apply design changes incrementally as long as the PRs don't introduce UI/UX issues. However, if you have concerns over shipping it (I will also only merge this PR after the release 21.7 code freeze is cut), I'm happy to create a feature branch and merge this PR to it.

@iamgabrielma iamgabrielma self-assigned this Feb 6, 2025
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These looks good! I haven't seen anything off while testing the normal cash/card/receipt flows, agree that we should make these in trunk rather than feature branch, so it's easier to catch if there's something off either with the colours or design-wise.

🚢

@@ -1,4 +1,5 @@
import Foundation
import SwiftUICore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 👀 was this a SwiftUI import maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: POS type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants