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: Language filter #2539

Merged
merged 15 commits into from
Jul 22, 2022
Merged

Conversation

abughalib
Copy link
Contributor

What

  • Dev mode language search

Screenshot

screen-20220707-233805.mp4

Part of

#2482

@abughalib abughalib requested a review from a team as a code owner July 7, 2022 20:11
@abughalib abughalib changed the title Language filter feat: Language filter Jul 7, 2022
@M123-dev M123-dev requested a review from AshAman999 July 7, 2022 23:07
Copy link
Member

@AshAman999 AshAman999 left a comment

Choose a reason for hiding this comment

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

Thanks for making that big effort to map the data to the language translations (+1 points for not using any librabry) and research about the supported languages for flutter.
Had some comments , If u could have a look.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @abughalib!
I'm sure the code can be improved; please have a look at my comments.

@teolemon
Copy link
Member

We have the theoretical possibility of using : https://static.openfoodfacts.org/data/taxonomies/languages.json

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2022

Codecov Report

Merging #2539 (7d5cfa1) into develop (2ea0da3) will decrease coverage by 1.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2539      +/-   ##
==========================================
- Coverage     8.86%   7.45%   -1.42%     
==========================================
  Files          161     214      +53     
  Lines         6623   10251    +3628     
==========================================
+ Hits           587     764     +177     
- Misses        6036    9487    +3451     
Impacted Files Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart 0.00% <0.00%> (-92.00%) ⬇️
...s/smooth_app/lib/data_models/user_preferences.dart 8.73% <0.00%> (-23.57%) ⬇️
packages/smooth_app/lib/themes/smooth_theme.dart 60.00% <0.00%> (-22.98%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.11% <0.00%> (-19.10%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 24.65% <0.00%> (-6.78%) ⬇️
packages/smooth_app/lib/main.dart 14.16% <0.00%> (-3.73%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (-0.88%) ⬇️
... and 226 more

@abughalib
Copy link
Contributor Author

abughalib commented Jul 11, 2022

screen-20220711-183438.mp4

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

That's much better @abughalib, thank you for your strong rewriting!
I still have several comments, please have a look at them.
In particular, I don't see the point of using your new Pair class: what you need is either a list of 2-character lang codes or even better a list of OpenFoodFactsLanguages. From it you can retrieve everything (name in English, name in language, 2-character code).

@abughalib
Copy link
Contributor Author

screen-20220720-210816.mp4

The search method now supports Name in English and language code in English.
The code is less efficient in terms of time complexity.

@abughalib abughalib requested a review from teolemon July 20, 2022 16:30
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @abughalib!

This is not the way I would have coded it but I must say that there were massive improvements compared to the early version and I assume that the code works.

We'll see later (in a future issue/PR) if changes are needed. Regarding performance or possible typos about the name in English.

[I haven't seen your latest changes]

@abughalib
Copy link
Contributor Author

@monsieurtanuki
I realized further performance optimization can be done this morning and I started working on it half hour ago.
Please let me know if I can make it even better.

Copy link
Member

@AshAman999 AshAman999 left a comment

Choose a reason for hiding this comment

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

Should be good to go for now, we can optimize it later if needed

@AshAman999
Copy link
Member

@M123-dev @teolemon
Can we move this language selection out of dev mode, might close as well #2503

@teolemon
Copy link
Member

@AshAman999 merging this one for now, and yes, we can then open a PR to make language selection for all (might require some other behavior changes, for data refresh)

@teolemon teolemon merged commit d856b35 into openfoodfacts:develop Jul 22, 2022
@AshAman999
Copy link
Member

Anyways, Congratulations on your 1st PR here ㊗️
And thanks @abughalib for the same

@abughalib
Copy link
Contributor Author

abughalib commented Jul 22, 2022

💯

@M123-dev
Copy link
Member

And sorry that it took so long @abughalib

@abughalib
Copy link
Contributor Author

@M123-dev
Not a Problem, Learned a lot during the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants