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

feat: 5201 - change currency with country when relevant #5238

Merged
merged 4 commits into from
May 11, 2024

Conversation

monsieurtanuki
Copy link
Contributor

What

We change the currency with the best possible matching value for a given country

  • at onboarding time for new users, silently
  • at app init time for old users that of course do not have a currency yet (recent field), silently
  • when the user explicitly changes the country, via a "confirm?" dialog

Screenshot

Screenshot_1715344369

Fixes bug(s)

Impacted files

  • app_en.arb: message for a "change currency too?" dialog
  • country_selector.dart: new method to change the currency at the same time as the country if relevant
  • product_query.dart: init currency if null
  • pubspec.lock: wtf
  • pubspec.yaml: upgraded openfoodfacts to 3.8.0
  • user_preferences_country_selector.dart: asking to change the currency at the same time as the country only if confirmed or relevant
  • welcome_page.dart: explicitly asking to change the currency at the same time as the country

Impacted files:
* `app_en.arb`: message for a "change currency too?" dialog
* `country_selector.dart`: new method to change the currency at the same time as the country if relevant
* `product_query.dart`: init currency if null
* `pubspec.lock`: wtf
* `pubspec.yaml`: upgraded `openfoodfacts` to `3.8.0`
* `user_preferences_country_selector.dart`: asking to change the currency at the same time as the country only if confirmed or relevant
* `welcome_page.dart`: explicitly asking to change the currency at the same time as the country
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner May 10, 2024 13:03
@github-actions github-actions bot added 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… dependencies labels May 10, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2024

Codecov Report

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

Project coverage is 8.62%. Comparing base (4d9c7fc) to head (83f4424).
Report is 111 commits behind head on develop.

Files Patch % Lines
...oth_app/lib/pages/onboarding/country_selector.dart 0.00% 22 Missing ⚠️
packages/smooth_app/lib/query/product_query.dart 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5238      +/-   ##
==========================================
- Coverage     9.54%   8.62%   -0.93%     
==========================================
  Files          325     329       +4     
  Lines        16411   16709     +298     
==========================================
- Hits          1567    1441     -126     
- Misses       14844   15268     +424     

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

@teolemon
Copy link
Member

teolemon commented May 10, 2024

Can we split into 2 sentences/strings, and avoid ellisions (You've) ?
Can we avoid adding things to the onboarding until we have a compelling value proposition ? (And even so, I'd rather move to small mono-theme, in context onboardings)

@monsieurtanuki
Copy link
Contributor Author

Can we split into 2 sentences/strings, and avoid ellisions (You've) ?

I can split into 2 sentences.
I'm not convinced by your idea of removing contractions - as explained here (https://m.youtube.com/watch?v=rNcS0S__WlQ), but as long as I split into 2 sentences you'll be able - I mean you will be able - to have it your way.

Can we avoid adding things to the onboarding until we have a compelling value proposition ? (And even so, I'd rather move to small mono-theme, in context onboardings)

I haven't really added anything to the onboarding. It's just that I doesn't seem relevant to display ADP (aka Andorran peseta) as the selected currency for any user that hasn't set the currency yet.
Or maybe I misinterpreted your comment.

@monsieurtanuki
Copy link
Contributor Author

@teolemon Done.

@monsieurtanuki monsieurtanuki merged commit 5f7966c into openfoodfacts:develop May 11, 2024
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score…
Development

Successfully merging this pull request may close these issues.

Try to prepopulate currency based on country for existing users
3 participants