-
Notifications
You must be signed in to change notification settings - Fork 130
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][Non-Simple Products] Update Variation Name Display Logic #13332
Conversation
📲 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13332 +/- ##
============================================
+ Coverage 41.10% 41.11% +0.01%
+ Complexity 6484 6481 -3
============================================
Files 1325 1325
Lines 77499 77527 +28
Branches 10669 10674 +5
============================================
+ Hits 31856 31876 +20
- Misses 42814 42819 +5
- Partials 2829 2832 +3 ☔ View full report in Codecov by Sentry. |
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 works well, @AnirudhBhat. I left a couple of questions – let me know what are your thoughts.
if (option?.option != null) { | ||
"${attribute.name}: ${option.option}" | ||
} else { | ||
"Any ${attribute.name}" |
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.
❓ Shouldn't this be localized?
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.
Currently, I believe "Any {attribute name}" is not localised in production as well. The reasoning is that "Any" functions like a generic dropdown option, similar to the other variation choices.
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.
Hm. I'm not sure I understand why we don't want to localise it as "Any %s".
"Any {attribute name}" is not localised in production
Do you refer to Woo wp-admin? I think texts in the app are localised independently of the web (e.g. you can have english version of Wordpress on server and the Woo app localised to the language of the OS).
It's also translated on iOS, as you can see here: https://github.com/woocommerce/woocommerce-ios/blame/trunk/Yosemite/Yosemite/Tools/ProductVariations/VariationAttributeViewModel.swift#L53
Let me know in case I'm missing something here @AnirudhBhat.
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.
@samiuelson Thanks for the review. I've added localisation only for the POS mode. Let me know what you think!
WooCommerce/src/main/kotlin/com/woocommerce/android/model/ProductVariation.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/model/ProductVariation.kt
Outdated
Show resolved
Hide resolved
…product attributes are available
…product attributes matches
…duct attributes matches
…then it returns 'Any {attribute}'
…n it returns 'Any {attribute}'
…OS is called, then it ignores those attributes
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.
Thanks for your work @AnirudhBhat. LGTM
Closes: #13330
Description
This PR updates the logic for displaying the name of variable products. Previously, only the attribute value (e.g., Red) was shown. With this change, both the attribute name and value (e.g., Color:Red) will be displayed for better clarity and user understanding.
Changes
getNameForPOS
function to include both attribute name and attribute value in the format {name}:{value}.Steps to reproduce
The tests that have been performed
Tested above mentioned steps in both light and dark mode
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: