woocommerce-android icon indicating copy to clipboard operation
woocommerce-android copied to clipboard

[Woo POS] Copy "woo" colors to our custom theme, as this is what we end up using for design

Open backwardstruck opened this issue 1 year ago • 3 comments
trafficstars

Closes: #12250

Description

Branched off of this small PR which can be reviewed as part of this change as well: https://github.com/woocommerce/woocommerce-android/pull/12193

Addresses the need to integrate the "Woo" color palette into our custom theme for design consistency. It copies the color definitions from the wc_colors_base.xml file into the custom theme, ensuring that both light and dark mode variants align with the prescribed design guidelines.

Steps to reproduce

  1. Verify that the colors align with the design specifications.
  2. Ensure there are no visual discrepancies or missing color assignments.

Testing information

  • Devices Used: Tablet emulator

Images/gif

No UI changes should be present.

  • [x] I have considered adding unit tests for this change. If I decided not to add them, I have provided a brief explanation below (optional):
    • Adding unit tests directly for theme colors is not practical. Visual verification is more appropriate in this context.
  • [x] I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

backwardstruck avatar Aug 09 '24 19:08 backwardstruck

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit201f1212a9960fadfeae91b194c4c2d43e23f1fa
Direct Downloadwoocommerce-wear-prototype-build-pr12275-201f121.apk

wpmobilebot avatar Aug 09 '24 19:08 wpmobilebot

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

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit201f1212a9960fadfeae91b194c4c2d43e23f1fa
Direct Downloadwoocommerce-prototype-build-pr12275-201f121.apk

wpmobilebot avatar Aug 09 '24 20:08 wpmobilebot

Codecov Report

Attention: Patch coverage is 0% with 137 lines in your changes missing coverage. Please review.

Project coverage is 40.58%. Comparing base (59a68a4) to head (201f121). Report is 52 commits behind head on trunk.

Files with missing lines Patch % Lines
.../android/ui/woopos/common/composeui/WooPosTheme.kt 0.00% 137 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #12275      +/-   ##
============================================
- Coverage     40.64%   40.58%   -0.07%     
  Complexity     5674     5674              
============================================
  Files          1229     1229              
  Lines         69178    69287     +109     
  Branches       9579     9579              
============================================
  Hits          28119    28119              
- Misses        38475    38584     +109     
  Partials       2584     2584              
Flag Coverage Δ
40.58% <0.00%> (-0.07%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Aug 09 '24 21:08 codecov-commenter

Version 20.2 has now entered code-freeze, so the milestone of this PR has been updated to 20.3.

wpmobilebot avatar Aug 30 '24 11:08 wpmobilebot

Version 20.3 has now entered code-freeze, so the milestone of this PR has been updated to 20.4.

wpmobilebot avatar Sep 06 '24 11:09 wpmobilebot

Thanks for this improvement! Please take a look into the comment I left. Wdyt?

Slack discussion with @joe-keenan who we asked for more details about POS colors.

Now that almost all the design TODOs are done, should I proceed with this one @kidinov @AnirudhBhat @samiuelson ? I'm just worried that this change will somehow cause bugs before the DM.

Sounds like the next steps would be:

  1. Copy-paste the "Woo" theme colors from wc_colors_base.xml and call them the way we can figure out which one is which in figma, e.g.: woo[Color][Number]

  2. Make this object private and part of the theme file, so it won't be possible to use non "themed" colors directly.

backwardstruck avatar Sep 17 '24 20:09 backwardstruck

@backwardstruck 👋

I'm just worried that this change will somehow cause bugs before the DM.

If we create a palette of colors and use the exact colors from the pallet in our theme colors, then IMO it's safe to implement

Your proposed steps make sense to me!

kidinov avatar Sep 19 '24 08:09 kidinov