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

Delete account view with basic bloc #296

Merged
merged 11 commits into from
Aug 26, 2024
Merged

Delete account view with basic bloc #296

merged 11 commits into from
Aug 26, 2024

Conversation

kackogut
Copy link
Collaborator

@kackogut kackogut commented Aug 1, 2024

Description

Please include a brief summary of what the issue is and what has been done to solve it

Fixes #<issue_number> or taiga link

Checklist:

Creator

  • The target branch is main
  • I have updated the unreleased section of the change log
  • I have reviewed the 'files changed' tab on github to ensure all changes are as expected
  • I have added someone to review the pr

Reviewer

  • I have verified the above are all completed
  • I have run the code locally to ensure it fufills the requirements of the issue
  • I have reviewed the 'files changed' and commented on any sections which I think are not needed, incorrect or could be improved

After pull

  • If appropriate I have closed the issue/ moved the trello card


await _userService.deleteUser();
await _authenticationService.logout();
_appRouter.pushAndPopUntil(const LoginRoute(), predicate: (r) => false);
Copy link
Member

Choose a reason for hiding this comment

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

Im not sure navigating the user to the login route is the right path here. If they are deleting their account Im guessing its more likely they don't want an account rather than they want to log into a different one. I could be wrong though... (e.g. maybe if the user wanted to change email or something but I think we can offer a different flow for that)

If you think that's right then we should probably leave them on the more menu page but have some kind of notification to show that the delete was successful... We might need to chat to design folks about that though.

appRouter: AutoRouter.of(context),
userService: locator<UserService>(),
),
child: BlocBuilder<DeleteAccountBloc, DeleteAccountState>(
Copy link
Member

Choose a reason for hiding this comment

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

More of a question, but is it generally better to have the bloc builders lower down in the tree (e.g. only wrapping the submit button and input) or is this ok? I assume the latter is better as it results in less re rendering but basically irrelevant for a page as simple as this but more generally is a good idea

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I recon so, I'm used to Android Compose way where only changed things are rebuilt, fixed thanks!

@kackogut kackogut force-pushed the feature/delete_account branch from 7f5d6a0 to 395c4e5 Compare August 8, 2024 20:40
@JElgar JElgar force-pushed the main branch 2 times, most recently from 08c843d to 69da324 Compare August 11, 2024 11:10
@kackogut kackogut marked this pull request as ready for review August 20, 2024 08:14
@JElgar JElgar force-pushed the feature/delete_account branch from f4441a8 to 24b5dfa Compare August 24, 2024 18:35
@JElgar JElgar force-pushed the feature/delete_account branch from bcc67ab to c6cd9c3 Compare August 24, 2024 18:54
@JElgar JElgar merged commit f8896d2 into main Aug 26, 2024
@JElgar JElgar deleted the feature/delete_account branch August 26, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants