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.

Sign up for GitHub

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

Jump to bottom

feat: improved app rating flow #3439

Merged
merged 9 commits into from
Dec 25, 2022
Merged

feat: improved app rating flow #3439

merged 9 commits into from
Dec 25, 2022

Conversation

Omegaviv
Copy link
Contributor

What

Refined the App rating flow issue #3420

Screenshot

Before

After

@Omegaviv Omegaviv requested a review from a team as a code owner December 13, 2022 10:27
@github-actions github-actions bot added 🌐 l10n 👥 User management Account login, signup, signout labels Dec 13, 2022
@Omegaviv
Copy link
Contributor Author

Let me know if any changes are suggested

@teolemon
Copy link
Member

Run flutter format --set-exit-if-changed .
Formatted packages/smooth_app/lib/pages/user_management/login_page.dart
Formatted 334 files (1 changed) in 1.53 seconds.
Formatting failed: 1
Error: Process completed with exit code 1.

@VaiTon
Copy link
Member

VaiTon commented Dec 17, 2022

@Omegaviv

 info • Prefer final for variable declarations if they are not reassigned • packages/smooth_app/lib/pages/user_management/login_page.dart:349:13 • prefer_final_locals
   info • Use Flutter TODO format: // TODO(username): message, https://URL-to-issue • packages/smooth_app/lib/pages/user_management/login_page.dart:382:19 • flutter_style_todos
   info • Avoid `print` calls in production code • packages/smooth_app/lib/pages/user_management/login_page.dart:383:19 • avoid_print
   info • Only use double quotes for strings containing single quotes • packages/smooth_app/lib/pages/user_management/login_page.dart:383:25 • prefer_single_quotes

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022 •
edited
Loading

Codecov Report

Merging #3439 (ef0ed42) into develop (cdac439) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3439      +/-   ##
===========================================
- Coverage    11.21%   11.18%   -0.04%     
===========================================
  Files          261      261              
  Lines        12631    12668      +37     
===========================================
  Hits          1417     1417              
- Misses       11214    11251      +37
Impacted Files Coverage Δ
...ooth_app/lib/pages/user_management/login_page.dart 45.55% <0.00%> (-11.79%) ⬇️

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

M123-dev
M123-dev reviewed Dec 21, 2022
View reviewed changes
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.

Heyyy @Omegaviv thanks for these changes. They look really good.

We should just give the user some way of giving negative feedback

packages/smooth_app/lib/pages/user_management/login_page.dart Outdated
positiveAction: SmoothActionButton(
text: appLocalizations
.app_rating_dialog_title_enjoying_positive_actions,
onPressed: () async => Navigator.of(context).pop(true),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onPressed: () async => Navigator.of(context).pop(true),
onPressed: () => Navigator.of(context).pop(true),

This doesn't need to be async

packages/smooth_app/lib/pages/user_management/login_page.dart Outdated
Comment on lines 382 to 383
// TODO(omegaviv): implement feedback form and link here,https://github.com/openfoodfacts/smooth-app/issues/3419
Navigator.of(context).pop(true);
Copy link
Member

Choose a reason for hiding this comment

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

Until this feedback form is implemented lets ask them to send a email to contact @ openfoodfacts.org. That should be quick to implement, we already have a package for this installed.

Otherwise we would ask them to give (negative) feedback but don't say how, so likely they will write a bad review

Copy link
Member

Choose a reason for hiding this comment

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

Or always open the english form for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should I redirect them to email directly at the press of a button?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sounds good

Copy link
Member

Choose a reason for hiding this comment

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

With a small comment, that the button is the way to add feedback

M123-dev
M123-dev reviewed Dec 24, 2022
View reviewed changes
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 @Omegaviv these changes look really great, please have a look at one very minor comment 😄

packages/smooth_app/lib/pages/user_management/login_page.dart Outdated
),
negativeAction: SmoothActionButton(
text: appLocalizations.not_really,
onPressed: () => Navigator.of(context).pop(false),
Copy link
Member

Choose a reason for hiding this comment

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

We don't use the poped false, do we

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

M123-dev
M123-dev approved these changes Dec 25, 2022
View reviewed changes
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, again thank you @Omegaviv

@M123-dev M123-dev merged commit 9c9b93b into openfoodfacts:develop Dec 25, 2022
@M123-dev
Copy link
Member

Congrats on your first PR here @Omegaviv 🎉

@github-actions github-actions bot mentioned this pull request Dec 25, 2022
Merged
@Omegaviv
Copy link
Contributor Author

Congrats on your first PR here @Omegaviv tada

Thank you @M123-dev

@M123-dev M123-dev mentioned this pull request Feb 11, 2023
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M123-dev M123-dev M123-dev approved these changes

Assignees

@Omegaviv Omegaviv

Labels
app rating 🌐 l10n 👥 User management Account login, signup, signout
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Omegaviv @teolemon @VaiTon @codecov-commenter @M123-dev @AshAman999