-
-
Notifications
You must be signed in to change notification settings - Fork 282
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: added the "Help improve OFF in your country" tile on the contribute page #5510
feat: added the "Help improve OFF in your country" tile on the contribute page #5510
Conversation
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!
That's good but there are still things to address. Please read my comments for details.
() async => LaunchUrlHelper.launchURL( | ||
'https://wiki.openfoodfacts.org/Country_Support_- ${userPreferences.userCountryCode ?? 'France'}'), | ||
const IconData(0xf68d), | ||
externalLink: true), |
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.
externalLink: true), | |
externalLink: true,), |
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.
It's not really optional.
'Help improve Open Food Facts in your country', | ||
() async => LaunchUrlHelper.launchURL( | ||
'https://wiki.openfoodfacts.org/Country_Support_- ${userPreferences.userCountryCode ?? 'France'}'), | ||
const IconData(0xf68d), |
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.
There's a globe icon you should reuse: the one used in the app country selector.
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.
You're right ill look
_getListTile( | ||
'Help improve Open Food Facts in your country', | ||
() async => LaunchUrlHelper.launchURL( | ||
'https://wiki.openfoodfacts.org/Country_Support_- ${userPreferences.userCountryCode ?? '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.
It's not the way it works:
- you need to maintain a Map of country/wiki-link, as stated in the issue, for the same limited list of countries, with the correct values
- if the current country is not in the Map, you mustn't display that ListTile.
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.
Okay will do
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5510 +/- ##
==========================================
- Coverage 9.54% 7.05% -2.50%
==========================================
Files 325 397 +72
Lines 16411 20889 +4478
==========================================
- Hits 1567 1473 -94
- Misses 14844 19416 +4572 ☔ 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, and never forget to format the code.
@@ -0,0 +1,25 @@ | |||
class CountryWikiLinks { |
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 call it something like TmpCountryWikiLinks
, and add a TODO
saying that the code is to be moved to openfoodfacts-dart.
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.
Will do. Also why am i adding the TODO saying the code will be moved to openfoodfacts-dart?
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.
Also why am i adding the TODO saying the code will be moved to openfoodfacts-dart?
Because part of the code (in our case, the map) will eventually be moved to openfoodfacts-dart.
When we code for Smoothie, we sometimes code things that can be reused by other openfoodfacts-dart users.
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.
Okay
), | ||
_getListTile( | ||
if(CountryWikiLinks().wikiLinks.containsKey(userPreferences.userCountryCode)) |
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.
Would be better to put the wiki link in a String?
variable before, and then to test if (wikiLinks != null)
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.
How come?
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.
Let me see if I understand are you saying put the TmpCountryWikiLinks()
.wikiLinks
.containsKey(userPreferences.userCountryCode) in a variable and test if this is going to be null
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.
If so how would that be better? Trying to learn best coding practices :)
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.
It's typically considered a best practice to avoid a code copy/paste.
Easier to maintain, easier to read, easier to understand.
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.
Where would I put the code because if I put it around line 57 it says userPreferences can't be accessed
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.
I'm sure you can put TmpCountryWikiLinks() .wikiLinks[userPreferences.userCountryCode]
in a local String?
variable.
If needed, have a look at tutorials specific to dart or flutter, for instance about the ways a method can return a value - something like =>
and return
syntaxes.
() async => LaunchUrlHelper.launchURL( | ||
'https://wiki.openfoodfacts.org/Country_Support_- ${userPreferences.userCountryCode ?? 'France'}'), | ||
const IconData(0xf68d), | ||
externalLink: true), |
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.
It's not really optional.
// 'Belgium': 'https://wiki.openfoodfacts.org/Country_Support_-_Belgium', | ||
// 'Bulgaria': 'https://wiki.openfoodfacts.org/Local_Communities/BulgarianTeam', |
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 don't put useless comments.
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.
I have harmonized on the first pattern FYI
packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart
Outdated
Show resolved
Hide resolved
@monsieurtanuki hey I was wondering what's the technical reason why we are using a map instead of just using strings is it just for organizing? |
I have no idea what you mean with "just using strings". Please use a Map for the moment: that's specifically something that would be refactored in openfoodfacts-dart anyway. |
/lint |
1 similar comment
/lint |
@jnnabugwu Still working on it? |
Sorry I’ve been wrapping up in preparing for interviews
…On Thu, Sep 12, 2024 at 10:18 monsieurtanuki ***@***.***> wrote:
@jnnabugwu <https://github.com/jnnabugwu> Still working on it?
—
Reply to this email directly, view it on GitHub
<#5510 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH3LYQZVAG3O7HD2SF6W6GDZWGPBVAVCNFSM6AAAAABLLJ32ZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBWGQZTCNJVGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@jnnabugwu No problem! |
b82769a
to
e3fa4f4
Compare
What
Screenshot
Fixes bug(s)