[Tour of Beam] [Frontend] Delete account, GitHub auth, solution warning by nausharipov · Pull Request #25347 · apache/beam

nausharipov · 2023-02-06T16:43:56Z

Resolves #25255
Resolves #25116
Resolves #25284

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

alexeyinkin · 2023-02-07T06:26:03Z

learning/tour-of-beam/frontend/lib/components/profile/user_menu.dart

+                  onContinue: () {
+                    authNotifier.deleteAccount().then(
+                      (_) {
+                        Navigator.pop(context);
+                      },
+                    );
+                  },


async / await.

Show progress?

Linter is against using context in async functions.

Do you mean showing an overlaying loading animation?

Oh, by the way this is just the same problem, but the linter cannot spot it. One solution is to let the dialog close itself by passing a callback to onContinue, but this is ugly. Try to think of better ideas. Also this is exactly why in app_state we have states pushing themselves out without a context.

Yes.

The simplest solution is to first pop the dialog and then to delete the account.

alexeyinkin · 2023-02-07T06:29:13Z

learning/tour-of-beam/frontend/lib/pages/tour/widgets/solution_button.dart

+          showDialog(
+            context: context,
+            builder: (context) => Dialog(
+              backgroundColor: Colors.transparent,
+              child: BeamAlertDialog(


Duplicate code.
BeamAlertDialog.show(context, ...); ?

Moved Dialog into BeamAlertDialog, because future users of the widget might not know about the

Future<void> show(BuildContext context) async { await showDialog( context: context, builder: build, ); }

function and use showDialog.

alexeyinkin · 2023-02-07T06:32:17Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+    this.body,
+    required this.continueLabel,
+    required this.onContinue,
+    required this.title,


Put required parameters first.

alexeyinkin · 2023-02-07T06:33:37Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+import '../../../playground_components.dart';
+
+class BeamAlertDialog extends StatelessWidget {
+  final String? body;


Suggested change

final String? body;

final String? text;

?

alexeyinkin · 2023-02-07T06:35:18Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+                  onPressed: () {
+                    Navigator.pop(context);
+                  },
+                  // TODO(nausharipov): review: translate in PGC?


Yes, we have easy_localization there already and slowly migrating strings there.

alexeyinkin · 2023-02-07T06:36:35Z

playground/frontend/playground_components/lib/src/widgets/popups/alert_dialog.dart

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+


Which of these two files is used?

Deleted /popups.

alexeyinkin · 2023-02-09T10:41:00Z

playground/frontend/playground_components/lib/src/widgets/overlay/opener.dart

  required BuildContext context,
  required PublicNotifier closeNotifier,
  required Positioned positioned,
+  bool isDismissible = true,


Why not false?

The current users of overlay (LoginButton and Avatar) expect it to be dismissible. Also, it is consistent with showDialog's bool barrierDismissible = true.

Rename it to barrierDismissible then?
I would prefer this to be required though because there is actually no good reason to prefer one over another.

I renamed the function to showOverlay and the parameter to barrierDismissible for consistency, still assuming that true is the preferable setting.

alexeyinkin · 2023-02-09T10:43:43Z

playground/frontend/playground_components/lib/src/widgets/overlay/widget.dart

    required this.close,
    required this.child,
+    required this.isDismissible,


alexeyinkin · 2023-02-09T10:43:58Z

playground/frontend/playground_components/lib/src/widgets/overlay/widget.dart

  final VoidCallback close;
  final Positioned child;
+  final bool isDismissible;


alexeyinkin · 2023-02-09T10:48:08Z

playground/frontend/playground_components/lib/src/widgets/overlay/overlays.dart

+  // TODO(nausharipov) review: add label?
+  // TODO(nausharipov) review: add grey-ish background?


alexeyinkin

LGTM (internal review).

nausharipov · 2023-02-09T15:25:58Z

R: @damondouglas

nausharipov · 2023-02-09T15:30:03Z

Switching between assignment and solution is done in #25298.

github-actions · 2023-02-09T16:05:32Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

damondouglas · 2023-02-09T16:39:27Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+
+import '../../../playground_components.dart';
+
+class BeamAlertDialog extends StatelessWidget {


Could you comment what this Widget does and why it exists? It's somewhat obvious that it is a general purpose dialog Widget but it would be helpful to others needing to change it in the future at least one sentence describing it.

Renamed the widget.

damondouglas · 2023-02-09T16:41:40Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+import '../../../playground_components.dart';
+
+class BeamAlertDialog extends StatelessWidget {
+  final String? text;


Comment that (I'm assuming) test lands in the body of the dialog widget? Why is it optional?

Renamed text to subtitle. It is optional, because some dialogs might not need body text.

damondouglas · 2023-02-09T16:41:53Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+
+class BeamAlertDialog extends StatelessWidget {
+  final String? text;
+  final String continueLabel;


What does continueLabel do? Can you add a comment?

Renamed the parameter.

damondouglas · 2023-02-09T16:42:13Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+  final String? text;
+  final String continueLabel;
+  final VoidCallback onContinue;
+  final String title;


Is there a character limit?

Wrapped the dialog content in SingleChildScrollView to avoid overflow.

damondouglas · 2023-02-09T16:42:34Z

playground/frontend/playground_components/lib/src/widgets/dialogs/alert_dialog.dart

+class BeamAlertDialog extends StatelessWidget {
+  final String? text;
+  final String continueLabel;
+  final VoidCallback onContinue;


Comment perhaps that the route is popped, etc.

Renamed the parameter.

damondouglas · 2023-02-09T16:44:25Z

playground/frontend/playground_components/lib/src/widgets/overlay/overlays.dart

+
+import '../../../playground_components.dart';
+
+class BeamOverlays {


The fact that it is plural stages future types of overlay widgets? Could you comment on the purpose of this class?

Added a comment.

damondouglas · 2023-02-09T16:44:59Z

playground/frontend/playground_components/lib/src/widgets/overlay/overlays.dart

+import '../../../playground_components.dart';
+
+class BeamOverlays {
+  static Future<void> showProgressOverlay(


I'm assuming the purpose of this is to simply overlay a CircularProgressIndicator? Could you add a comment?

Added a comment.

nausharipov · 2023-02-10T15:47:50Z

R: @damondouglas

damondouglas

@olehborysevych Are you able to update the tour of beam readme backend instructions so I can review this as an actual running application?

alexeyinkin · 2023-02-14T12:08:16Z

playground/frontend/playground_components/lib/src/widgets/dialogs/progress.dart

 import 'package:flutter/material.dart';

-import '../../../playground_components.dart';
+class ProgressDialog extends StatelessWidget {


Move the comment from the overlay.

darkhan.nausharipov added 3 commits February 6, 2023 21:03

text and popup (apache#25116)

f9f8c3a

hint warning (apache#25116)

a34b170

delete my account (apache#25255)

eda0bfe

github-actions bot added the learning label Feb 6, 2023

darkhan.nausharipov added 2 commits February 6, 2023 22:51

pre-review fixes (apache#25255)

d67cf47

todo (apache#25255)

bc4d930

alexeyinkin suggested changes Feb 7, 2023

View reviewed changes

darkhan.nausharipov added 4 commits February 7, 2023 14:51

comment fixes (apache#25255)

17b8c42

GitHub auth untested (apache#25255)

2ab46ec

showProgressOverlay (apache#25255)

eda3f16

toastNotifier todo (apache#25255)

8c3e4b5

nausharipov requested a review from alexeyinkin February 9, 2023 08:42

alexeyinkin suggested changes Feb 9, 2023

View reviewed changes

darkhan.nausharipov added 2 commits February 9, 2023 19:54

comment fixes (apache#25255)

f1a2cec

BeamRouterDelegate for showing toasts (apache#25255)

a2f43bf

nausharipov requested a review from alexeyinkin February 9, 2023 14:02

addException in deleteAccount (apache#25255)

09a5526

alexeyinkin approved these changes Feb 9, 2023

View reviewed changes

showOverlay (apache#25255)

fde6ace

nausharipov changed the title ~~[Tour of Beam] [Frontend] Delete account~~ [Tour of Beam] [Frontend] Delete account, GitHub auth, solution warning Feb 9, 2023

nausharipov marked this pull request as ready for review February 9, 2023 15:25

damondouglas requested changes Feb 9, 2023

View reviewed changes

addressing review comments (apache#25255)

6005103

nausharipov requested a review from alexeyinkin February 10, 2023 09:14

darkhan.nausharipov added 2 commits February 10, 2023 21:17

addressing review comments (apache#25255)

440d0b0

addressing review (apache#25255)

12882fb

nausharipov removed the request for review from alexeyinkin February 10, 2023 15:46

nausharipov requested a review from damondouglas February 10, 2023 15:46

damondouglas requested changes Feb 11, 2023

View reviewed changes

darkhan.nausharipov added 3 commits February 13, 2023 22:40

pop progress dialog with navigatorKey (apache#25255)

7626f8f

updated to app_state 0.9.2 (apache#25255)

2cb7c58

progress dialog is undismissible (apache#25255)

a5110e4

alexeyinkin suggested changes Feb 14, 2023

View reviewed changes

comment fixes (apache#25255)

3f3747c

nausharipov requested a review from alexeyinkin February 14, 2023 12:29

alexeyinkin approved these changes Feb 14, 2023

View reviewed changes

darkhan.nausharipov added 3 commits February 22, 2023 21:34

Merge branch 'master' into issue25255-delete-account

dc44ba7

flutter v3.7 support (apache#25255)

6b8ba1a

Merge branch 'master' into issue25255-delete-account

9f42763

nausharipov closed this Mar 28, 2023

nausharipov mentioned this pull request Mar 29, 2023

[Tour of Beam] Save code, Delete account & other tasks #26010

Merged

3 tasks

nausharipov deleted the issue25255-delete-account branch May 15, 2023 14:49

		// TODO(nausharipov) review: add label?
		// TODO(nausharipov) review: add grey-ish background?


		import '../../../playground_components.dart';

		class BeamAlertDialog extends StatelessWidget {


		import '../../../playground_components.dart';

		class BeamOverlays {

Conversation

nausharipov commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nausharipov Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nausharipov Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeyinkin left a comment

Choose a reason for hiding this comment

Uh oh!

nausharipov commented Feb 9, 2023

Uh oh!

nausharipov commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nausharipov Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nausharipov Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nausharipov Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nausharipov Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nausharipov commented Feb 6, 2023 •

edited

Loading

nausharipov Feb 7, 2023 •

edited

Loading

nausharipov Feb 7, 2023 •

edited

Loading

nausharipov commented Feb 9, 2023 •

edited

Loading

nausharipov Feb 10, 2023 •

edited

Loading

nausharipov Feb 10, 2023 •

edited

Loading

nausharipov Feb 10, 2023 •

edited

Loading

nausharipov Feb 10, 2023 •

edited

Loading

nausharipov Feb 10, 2023 •

edited

Loading