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: Add btn to finish adding new product #4706

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

WildOrangutan
Copy link
Contributor

What

Add btn on the end of product page, when adding a new product.

Marking as draft for now - waiting #4698 to merge and resolve conflicts after.

Screenshot

image

Fixes bug(s)

Fixes #4670

@github-actions github-actions bot added 🥫 Product page Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🌐 l10n labels Oct 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2023

Codecov Report

Merging #4706 (069fb12) into develop (25068e5) will decrease coverage by 0.08%.
Report is 29 commits behind head on develop.
The diff coverage is 2.50%.

@@            Coverage Diff             @@
##           develop   #4706      +/-   ##
==========================================
- Coverage     9.90%   9.83%   -0.08%     
==========================================
  Files          310     313       +3     
  Lines        15808   15916     +108     
==========================================
- Hits          1566    1565       -1     
- Misses       14242   14351     +109     
Files Coverage Δ
...mooth_app/lib/generic_lib/widgets/smooth_card.dart 78.94% <100.00%> (+1.16%) ⬆️
...th_app/lib/pages/product/add_new_product_page.dart 0.00% <0.00%> (ø)

... and 30 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@M123-dev
Copy link
Member

Heyy @WildOrangutan the screenshot looks good to me, why is this Pull Request wip

@WildOrangutan
Copy link
Contributor Author

@M123-dev it's ready. I marked it as a draft, because you said to wait for the other MR 🙂

I will mark it as ready.

@WildOrangutan WildOrangutan marked this pull request as ready for review October 20, 2023 22:40
@WildOrangutan WildOrangutan requested a review from a team as a code owner October 20, 2023 22:40
Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Heyy @WildOrangutan sorry for the late review, I added a small comment regarding the translation string, otherwise the code looks good to me. Feel free to ping me when its fixed then we can merge before the October ends

Comment on lines 500 to 501
OpenFoodAPIConfiguration.globalUser?.userId ??
localizations.user_profile_title_guest),
Copy link
Member

Choose a reason for hiding this comment

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

"user_profile_title_guest" will just be Welcome!

The sentence

Thanks for your contribution "welcome!"! doesn't make sense.
I'd suggest to create a new no user string in app_en.arb

Something like

"new_product_done_msg_no_user": "Thanks for your contribution !",

Comment on lines 560 to 568
String? _getRegisteredUserName() {
final String? userId = OpenFoodAPIConfiguration.globalUser?.userId;
if (userId == null || userId.toLowerCase().contains('welcome')) {
return null;
} else {
return userId;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any more appropriate place to put this helper function?

Copy link
Member

Choose a reason for hiding this comment

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

Don't think so, we have a similar code for the settings page but its fine to leave it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will leave it here for now. I guess it could be moved to UserManagementHelper, so we can remove this duplicate logic.

@WildOrangutan
Copy link
Contributor Author

Heyy @WildOrangutan sorry for the late review, I added a small comment regarding the translation string, otherwise the code looks good to me. Feel free to ping me when its fixed then we can merge before the October ends

@M123-dev no worries. I've updated with suggested changes. Please check again.

Comment on lines 560 to 568
String? _getRegisteredUserName() {
final String? userId = OpenFoodAPIConfiguration.globalUser?.userId;
if (userId == null || userId.toLowerCase().contains('welcome')) {
return null;
} else {
return userId;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't think so, we have a similar code for the settings page but its fine to leave it here

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Perfect, thanks a lot @WildOrangutan

@M123-dev M123-dev merged commit 9f713f5 into openfoodfacts:develop Oct 31, 2023
6 checks passed
@WildOrangutan WildOrangutan deleted the 4670/btn branch October 31, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌐 l10n Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fast track: add a button to close the edit page
3 participants