Skip to content

Conversation

@momshaddinury
Copy link
Contributor

Summary

This MR aims to close #84 .

Other Information

Some unused code was removed.

@momshaddinury momshaddinury added enhancement New feature or request refactoring labels Sep 13, 2022
@momshaddinury momshaddinury requested a review from a team as a code owner September 13, 2022 04:22
@momshaddinury momshaddinury self-assigned this Sep 13, 2022
@tenshiAMD tenshiAMD requested a review from jorre127 September 16, 2022 15:53
Copy link
Contributor

@tenshiAMD tenshiAMD left a comment

Choose a reason for hiding this comment

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

@dinurymomshad hi, can you rebase and check if all works fine? thanks

required this.setExerciseName,
});

final Function(String) setExerciseName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add return type and name of the parameter, so final void Function(String name) setExerciseName;

// set up the AlertDialog
RedoWorkoutAlert alert = RedoWorkoutAlert(viewButton, redoButton);
Future<void> _showViewOrRedoAlertDialog(BuildContext context) async {
if (await showViewOrRedoAlertDialog(context)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe work with an enum here that is returned instead of a bool, because now its not entirely clear when "true" is returned for example if this means a redo or a view.

Copy link
Contributor

@jorre127 jorre127 Sep 16, 2022

Choose a reason for hiding this comment

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

Work with guard clauses when you can to make code less cluttered and more readable, here for example:

  Future<void> _showViewOrRedoAlertDialog(BuildContext context) async {
    if (!await showViewOrRedoAlertDialog(context)) return addExercises(widget.workout);
    Navigator.of(context).pushReplacement(
      MaterialPageRoute(
        builder: (context) => WorkoutPage(widget.workout),
      ),
    );
  }

);
/// Alert Dialogs
void _showAddNewExerciseDialog(BuildContext context) async {
ExerciseInputField exerciseNameSelectionWidget =
Copy link
Contributor

Choose a reason for hiding this comment

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

Use final for fields that don't change + guard clause, also be consistent when you want to declare variables, exerciseNameSelection widget gets stored into a variable before being used in the dialog, but the SaveWorkoutContent widget just gets send directly to the dialog.

  void _showAddNewExerciseDialog(BuildContext context) async {
    final exerciseNameSelectionWidget = ExerciseInputField(setExerciseName: setExerciseName);

    if (!await showAddNewExerciseDialog(context, exerciseNameSelectionWidget)) return;
    addExercise();
  }

required this.title,
this.subtitle,
this.content,
required this.buttons,
Copy link
Contributor

Choose a reason for hiding this comment

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

Put required parameters first

final Map<String, T> buttons;
}

extension Present<T> on AlertDialogModel<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Name it something like PresentExtension so you know its an extension from the name.

return ThemeProvider(builder: (context, AppTheme theme) {
return AlertDialog(
backgroundColor: theme.colorTheme.backgroundColorVariation,
titleTextStyle: Theme.of(context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Store the theme in a variable at the top of this method so you don't have to call .of(context) every time.

backgroundColor: theme.colorTheme.backgroundColorVariation,
titleTextStyle: Theme.of(context)
.textTheme
.headline6
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for this PR immediately, but we should store the different textThemes in the theme provided by themeProvider ass well so we don't have 2 themes.

context: context,
barrierDismissible: barrierDismissible,
builder: (context) {
return ThemeProvider(builder: (context, AppTheme theme) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Defining the type of theme here isn't necessary .

}

Future<bool> showDeleteWorkoutDialog(BuildContext context) {
return const _DeleteDialog(objName: 'workout').present(context).then(
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I think using await is clearer and looks better than using then + const isn't needed here since you're not storing the dialog. Same for the rest of the dialogs (the await not the const)

Future<bool> showDeleteWorkoutDialog(BuildContext context) async {
  final result = await _DeleteDialog(objName: 'workout').present(context);
  return result ?? false;
} 

Or maybe better

Future<bool> showDeleteWorkoutDialog(BuildContext context) async => await _DeleteDialog(objName: 'workout').present(context) ?? false;


Future<bool> showAddNewExerciseDialog(BuildContext context, Widget? content) {
return _AddNewExerciseDialog(
objName: 'Exercise',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe have the objName as an optional parameter to the showAddNewExerciesDialog method because otherwise I don't really see the reason it isn't just hardcoded in the constructor above, same for the rest of the dialogs.

@stale
Copy link

stale bot commented Oct 1, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the stale label Oct 1, 2022
@tenshiAMD tenshiAMD added pinned and removed stale labels Oct 1, 2022
@mergeable
Copy link

mergeable bot commented Oct 1, 2022

Lines of code have too many changes. It should be under 500 lines of addtions and deletions.

1 similar comment
@mergeable
Copy link

mergeable bot commented Oct 1, 2022

Lines of code have too many changes. It should be under 500 lines of addtions and deletions.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simplify/Create the AlertDialog API

4 participants