-
-
Notifications
You must be signed in to change notification settings - Fork 299
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: Contribute to your country link #5874
base: develop
Are you sure you want to change the base?
feat: Contribute to your country link #5874
Conversation
…jnnabugwu/smooth-app into contribute-to-your-country-link
…tribute.dart Co-authored-by: monsieurtanuki <[email protected]>
…jnnabugwu/smooth-app into contribute-to-your-country-link
…jnnabugwu/smooth-app into contribute-to-your-country-link
…our-country-link' of https://github.com/jnnabugwu/smooth-app into contribute-to-your-country-link
…jnnabugwu/smooth-app into contribute-to-your-country-link
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5874 +/- ##
==========================================
- Coverage 9.54% 6.85% -2.70%
==========================================
Files 325 428 +103
Lines 16411 23598 +7187
==========================================
+ Hits 1567 1617 +50
- Misses 14844 21981 +7137 ☔ 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.
Hi @jnnabugwu!
Please have a look at my comments!
@@ -319,6 +335,19 @@ class UserPreferencesContribute extends AbstractUserPreferences { | |||
builder: (_) => tile, | |||
); | |||
} | |||
|
|||
Future<String> returnCountry(BuildContext context) async { |
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.
Please remove that method, we don't need it.
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.
Whoops yeah thats my bad
@@ -0,0 +1,25 @@ | |||
// TODO(monsieurtanuki): the code is to be moved to openfoodfacts-dart | |||
class TmpCountryWikiLinks { | |||
final Map<String, String> wikiLinks = <String, String>{ |
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.
final Map<String, String> wikiLinks = <String, String>{ | |
static final Map<OpenFoodFactsCountry, String> wikiLinks = <OpenFoodFactsCountry, String>{ |
@@ -94,6 +96,20 @@ class UserPreferencesContribute extends AbstractUserPreferences { | |||
() async => _share(appLocalizations.contribute_share_content), | |||
Icons.adaptive.share, | |||
), | |||
_getListTile( |
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.
We display nothing if the country has no wiki links.
_getListTile( | ||
appLocalizations.help_improve_country, | ||
() async { | ||
final String country = await returnCountry(context); |
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.
Please use ProductQuery.getCountry() instead.
'Canada': 'https://wiki.openfoodfacts.org/Country_Support_-_Canada', | ||
'China': 'https://wiki.openfoodfacts.org/Country_Support_-_China', | ||
'Europe': 'https://wiki.openfoodfacts.org/Country_Support_-_Europe', | ||
'France': |
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.
Can I delete Europe?
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.
Can I delete Europe?
I can think of at least 2 countries leaders that already plan to do that and they don't need your help...
Back to the code now: in that particular case, we deal with countries so we don't need the "Europe" link.
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.
Hi @jnnabugwu!
Thanks for the latest changes in your code!
We need your code to be "dart-formatted", and it's currently not the case.
In Android Studio for instance, in the settings you can let dart
reformat your code when you type Ctrl-S. Very convenient.
Please have your code formatted and push the related changes.
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.
Hi @jnnabugwu!
You're not supposed to have warnings either:
warning • Unused import: 'package:iso_countries/iso_countries.dart' • packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart:7:8 • unused_import
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.
Hi @jnnabugwu!
Looks better, please have a look at my comments.
In addition to that, unluckily for you the page you're working on is part of the pages we control - UI-wise - to check if something changed.
Obviously something changed as you added an item.
So you have to run the following line, that will create the test standard pages as png files, that you'll have to include in your PR:
flutter test --update-goldens
(not even sure of the syntax)
class TmpCountryWikiLinks { | ||
final Map<OpenFoodFactsCountry, String> wikiLinks = | ||
<OpenFoodFactsCountry, String>{ | ||
OpenFoodFactsCountry.ALGERIA: |
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.
OpenFoodFactsCountry.ALGERIA: | |
OpenFoodFactsCountry.ARGENTINA: |
@@ -49,6 +50,10 @@ class UserPreferencesContribute extends AbstractUserPreferences { | |||
@override | |||
Color? getHeaderColor() => const Color(0xFFFFF2DF); | |||
|
|||
OpenFoodFactsCountry country = ProductQuery.getCountry(); |
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.
country
and links
should rather be final
variables inside method getChildren
: nowhere else in the code will we need them, and we don't want to give a wrong impression to the next code maintainers.
That means you'll have to switch getChildren
from the =>
short syntax to the return
syntax.
What
feat: Added a method and map to get the correct country locale for the corresponding wiki page.
Screenshot
Fixes bug(s)
Part of