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

Fixed double entry bug when clicking safe homework button again after uploading #1699

Merged
merged 4 commits into from
Aug 10, 2024

Conversation

HumanBot000
Copy link
Contributor

Fixed bug #1117 and fixed some grammar mistakes in CONTRIBUTING.md

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2024

CLA assistant check
All committers have signed the CLA.

@Jonas-Sander Jonas-Sander added the safe to test Used for fork PRs when the code is safe to test to trigger our CI. label Aug 9, 2024
@github-actions github-actions bot removed the safe to test Used for fork PRs when the code is safe to test to trigger our CI. label Aug 9, 2024
Copy link

github-actions bot commented Aug 9, 2024

Visit the preview URL for this PR (updated for commit cc53a53):

https://sharezone-test--pr1699-double-homeworks-saf-cbxxkhdk.web.app

(expires Tue, 13 Aug 2024 10:49:56 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b

Copy link
Collaborator

@Jonas-Sander Jonas-Sander left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your help! 💙

Right now I can't merge the PR since there is a warning from the Dart analyzer.

To fix this you could either change the _SaveButton to a StatefulWidget or use a StatefulBuilder like shown here:

  @override
  Widget build(BuildContext context) {
    bool isSaving = false;
    return StatefulBuilder(builder: (context, setState) {
      return SaveButton(
        key: HwDialogKeys.saveButton,
        tooltip: "Hausaufgabe speichern",
        onPressed: isSaving
            ? null
            : () {
                setState(() => isSaving = true);
                onPressed(context);
              },
      );
    });
  }

I would recommend using the StatefulBuilder in this case. Additionally I disabled the save button in the case above instead of showing a SnackBar. I think its nicer this way.

Lastly I would be grateful if you could extract the changes to the CONTRIBUTING.md into a seperate PR.

Thanks! :)

Comment on lines 339 to 341
class _SaveButton extends StatelessWidget {
const _SaveButton({this.editMode = false});
bool _homeworkIsSaving = false;
_SaveButton({this.editMode = false});

Copy link
Collaborator

@Jonas-Sander Jonas-Sander Aug 10, 2024

Choose a reason for hiding this comment

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

Since it's a stateless class you can't (or you shouldn't) use a mutable variable. This causes a warning to be shown, which is why the analyzer CI step is failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@HumanBot000
Copy link
Contributor Author

Hey, thanks for your help! 💙

Right now I can't merge the PR since there is a warning from the Dart analyzer.

To fix this you could either change the _SaveButton to a StatefulWidget or use a StatefulBuilder like shown here:

  @override
  Widget build(BuildContext context) {
    bool isSaving = false;
    return StatefulBuilder(builder: (context, setState) {
      return SaveButton(
        key: HwDialogKeys.saveButton,
        tooltip: "Hausaufgabe speichern",
        onPressed: isSaving
            ? null
            : () {
                setState(() => isSaving = true);
                onPressed(context);
              },
      );
    });
  }

I would recommend using the StatefulBuilder in this case. Additionally I disabled the save button in the case above instead of showing a SnackBar. I think its nicer this way.

Lastly I would be grateful if you could extract the changes to the CONTRIBUTING.md into a seperate PR.

Thanks! :)

Are you sure that showing nothing is the better way?

@Jonas-Sander
Copy link
Collaborator

Hey, thanks for your help! 💙
Right now I can't merge the PR since there is a warning from the Dart analyzer.
To fix this you could either change the _SaveButton to a StatefulWidget or use a StatefulBuilder like shown here:

  @override
  Widget build(BuildContext context) {
    bool isSaving = false;
    return StatefulBuilder(builder: (context, setState) {
      return SaveButton(
        key: HwDialogKeys.saveButton,
        tooltip: "Hausaufgabe speichern",
        onPressed: isSaving
            ? null
            : () {
                setState(() => isSaving = true);
                onPressed(context);
              },
      );
    });
  }

I would recommend using the StatefulBuilder in this case. Additionally I disabled the save button in the case above instead of showing a SnackBar. I think its nicer this way.
Lastly I would be grateful if you could extract the changes to the CONTRIBUTING.md into a seperate PR.
Thanks! :)

Are you sure that showing nothing is the better way?

Yes, I tested it locally and I liked it better. Since there is a "sending data..." SnackBar anyways, the user knows that the data is in transit.

@HumanBot000
Copy link
Contributor Author

Hey, thanks for your help! 💙
Right now I can't merge the PR since there is a warning from the Dart analyzer.
To fix this you could either change the _SaveButton to a StatefulWidget or use a StatefulBuilder like shown here:

  @override
  Widget build(BuildContext context) {
    bool isSaving = false;
    return StatefulBuilder(builder: (context, setState) {
      return SaveButton(
        key: HwDialogKeys.saveButton,
        tooltip: "Hausaufgabe speichern",
        onPressed: isSaving
            ? null
            : () {
                setState(() => isSaving = true);
                onPressed(context);
              },
      );
    });
  }

I would recommend using the StatefulBuilder in this case. Additionally I disabled the save button in the case above instead of showing a SnackBar. I think its nicer this way.
Lastly I would be grateful if you could extract the changes to the CONTRIBUTING.md into a seperate PR.
Thanks! :)

Are you sure that showing nothing is the better way?

Yes, I tested it locally and I liked it better. Since there is a "sending data..." SnackBar anyways, the user knows that the data is in transit.

Okay

@Jonas-Sander Jonas-Sander added the safe to test Used for fork PRs when the code is safe to test to trigger our CI. label Aug 10, 2024
@github-actions github-actions bot removed the safe to test Used for fork PRs when the code is safe to test to trigger our CI. label Aug 10, 2024
@Jonas-Sander Jonas-Sander added the safe to test Used for fork PRs when the code is safe to test to trigger our CI. label Aug 10, 2024
@github-actions github-actions bot removed the safe to test Used for fork PRs when the code is safe to test to trigger our CI. label Aug 10, 2024
Copy link
Collaborator

@Jonas-Sander Jonas-Sander left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you :)

@Jonas-Sander Jonas-Sander added this pull request to the merge queue Aug 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 10, 2024
@Jonas-Sander Jonas-Sander added this pull request to the merge queue Aug 10, 2024
Merged via the queue into SharezoneApp:main with commit 44e8401 Aug 10, 2024
39 of 52 checks passed
Comment on lines +373 to +374
setState(() => isSaving = true);
onPressed(context);
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this causes a bug if an error occurs during the upload (e.g. network issue) because then the save button is still disabled and the user can't retry.

I think the following change could fix it:

setState(() => isSaving = true);
await onPressed(context);
setState(() => isSaving = false);

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

Successfully merging this pull request may close these issues.

4 participants