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

Blaze: Ad target languages #10641

Merged
merged 10 commits into from
Jan 30, 2024
Merged

Blaze: Ad target languages #10641

merged 10 commits into from
Jan 30, 2024

Conversation

0nko
Copy link
Contributor

@0nko 0nko commented Jan 27, 2024

Adresses #10589. The PR implements the target language selection in the ad preview screen.

Screen_recording_20240127_150953.webm

To test:

  1. Start the Blaze campaign creation
  2. Select a product
  3. Tap on the Languages option
  4. Notice a language selection screen opens
  5. Select some languages
  6. Tap on the Save button
  7. Notice the selected languages are displayed in the preview

Note: Please merge #10638 before merging this.

@0nko 0nko added feature: blaze Related to the Blaze project unit-tests-exemption labels Jan 27, 2024
@0nko 0nko added this to the 17.4 milestone Jan 27, 2024
@dangermattic
Copy link
Collaborator

3 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class BlazeCampaignTargetSelectionViewModel is missing tests, but unit-tests-exemption label was set to ignore this.
⚠️ Class BlazeTargetType is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

📲 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
FlavorJalapeno
Build TypeDebug
Commitec280ac
Direct Downloadwoocommerce-prototype-build-pr10641-ec280ac.apk

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2024

Codecov Report

Attention: 69 lines in your changes are missing coverage. Please review.

Comparison is base (60a330a) 41.41% compared to head (ec280ac) 41.37%.
Report is 30 commits behind head on trunk.

Files Patch % Lines
...n/targets/BlazeCampaignTargetSelectionViewModel.kt 0.00% 41 Missing ⚠️
...n/preview/BlazeCampaignCreationPreviewViewModel.kt 0.00% 21 Missing ⚠️
...droid/ui/blaze/creation/targets/BlazeTargetType.kt 0.00% 4 Missing ⚠️
...om/woocommerce/android/ui/blaze/BlazeRepository.kt 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #10641      +/-   ##
============================================
- Coverage     41.41%   41.37%   -0.05%     
  Complexity     4979     4979              
============================================
  Files          1007     1009       +2     
  Lines         57720    57780      +60     
  Branches       7678     7683       +5     
============================================
  Hits          23904    23904              
- Misses        31698    31758      +60     
  Partials       2118     2118              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JorgeMucientes JorgeMucientes self-assigned this Jan 28, 2024
Base automatically changed from blaze/refactor-preview-viewstate to trunk January 29, 2024 14:28
Comment on lines +52 to +53
languages,
selectedLanguages
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor np, Wdyt about having a single flow for languages and add a new boolean field to the Language model to save the isSelected state. I feel with this approach of 2 different flows one for all the values and one for the selected ones we are going to have an explosion of flows once we add the rest of the campaign details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm.. 🤔 I'm not convinced this would be cleaner, in the end. We'd save 4 flows overall but updating the selection and returning results would result in copying the entire lists and mapping, like this:

languages.update { items -> items.map { it.copy(isSelected = it.language.code in selectedIds) } } } instead of selectedLanguages.update { selectedIds }.

Let's discuss this further, I don't feel too strongly about this.. 🙂

@JorgeMucientes
Copy link
Contributor

Great work @0nko, code looks good and works as expected. I left a minor suggestion, but there's no need to apply it in this PR or at all. So feel free to merge whenever you prefer.

@0nko 0nko merged commit 91fce81 into trunk Jan 30, 2024
16 of 22 checks passed
@0nko 0nko deleted the issue/10589-language-selection branch January 30, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: blaze Related to the Blaze project unit-tests-exemption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants