Fixes #4294: Profile successfully deleted message #4440
Conversation
|
@mbobiosio is this ready for review? If so, please make sure to assign it to whoever needs to review it (per https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section you should make sure that the 'assignees' list always includes whoever next has to take action on the PR). If it's not ready for review, please mark it as 'Draft' until it is ready to go into review. |
There was a problem hiding this comment.
Thanks @mbobiosio! I took an initial high-level pass. My feedback is:
- Please update the PR description to include details of what your PR contains, and how you approached it. PRs like Fix #3622, #4238, #3861, part of #4044: Fix state player deadlock by migrating progress controllers over to a command-queue structure #4239 are good examples of detailed descriptions, though that PR's description is much longer than what's probably needed here. Make sure to also fill in the 'UI' section per the instructions since this is introducing a new UI.
- We shouldn't be using a toast here. Generally, we probably should be using a snackbar, but since we aren't yet utilizing those yet this should be an alert dialog hosted in its own dialog fragment (as we do for other similar notices in the app).
- New tests should be added for changed functionality to make sure the notice appears with the correct text at the correct time.
| @@ -101,15 +103,16 @@ class ProfileEditFragmentPresenter @Inject constructor( | |||
| ProfileId.newBuilder().setInternalId(internalProfileId).build(), | |||
| binding.profileEditAllowDownloadSwitch.isChecked | |||
| ).toLiveData().observe( | |||
There was a problem hiding this comment.
Could probably fit on one line now for a bit cleaner code, e.g.: ).toLiveData().observe(activity) {
| fragment | ||
| ) { | ||
| if (it is AsyncResult.Success) { | ||
| fragment.requireContext().toast( |
There was a problem hiding this comment.
@BenHenning For accessibility perspective should we call this Toast as soon as it appears?
| AppVersionActivity::class.java, /* initialTouchMode= */ true, /* launchActivity= */ false | ||
| AppVersionActivity::class.java, /* initialTouchMode= */ | ||
| true, /* launchActivity= */ | ||
| false |
There was a problem hiding this comment.
These changes can be un-done.
AppVersionActivity::class.java, /* initialTouchMode= */ true, /* launchActivity= */ false
|
@mbobiosio I'll need to close this since it was force-pushed. Please review https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR (particularly point 4 under https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#making-a-local-code-change-using-the-terminal). We don't allow force-pushing since it messes up comment history and can make code reviewing much more difficult. You can open a new PR with a fresh history, just please make sure not to force push again in the future as it will lead to delays in the review process. Thanks! |
Explanation
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: