-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix #4028: Hi fi profile tablet implement edit profile screen inside a fragment #4029
Fix #4028: Hi fi profile tablet implement edit profile screen inside a fragment #4029
Conversation
added logic by implementing listeners and handlers for ProfileEditFragment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @BenHenning I tried to implement a back stack management so that we can add fragments on the back stack and if back button is pushed then we can simply pop the fragment but this isn't working.
first I tired adding tags tags were properly displayed on the fragments list as I logged it but the backstack size was coming zero
this is the output below for my above run
2021-12-10 14:48:14.134 9140-9140/org.oppia.android I/tag: handleOnBackPressed: ProfileEditFragment{e213891} (224a34e6-8455-4819-805b-bcd6b5be6ba7) id=0x7f090072 ADMINISTRATOR_CONTROLS_BACKSTACK} [NavigationDrawerFragment{48773f6} (fe0c3145-91f3-4383-bed9-d71069c05b9b) id=0x7f090070}, AdministratorControlsFragment{94583f7} (4de28ea0-b6e0-4576-b7c5-d7d3423e41a6) id=0x7f090073 ADMINISTRATOR_CONTROLS_BACKSTACK}, ProfileListFragment{8d8a264} (ba3d3f5a-c561-4f96-bb9f-355b9940abc3) id=0x7f090072 ADMINISTRATOR_CONTROLS_BACKSTACK}, SupportRequestManagerFragment{30fcccd} (9a4b0150-e5f1-4b4c-84dc-4e9555ffcd51) com.bumptech.glide.manager}{parent=null}, ProfileEditFragment{e213891} (224a34e6-8455-4819-805b-bcd6b5be6ba7) id=0x7f090072 ADMINISTRATOR_CONTROLS_BACKSTACK}] 0
...n/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityPresenter.kt
Outdated
Show resolved
Hide resolved
...n/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityPresenter.kt
Outdated
Show resolved
Hide resolved
...n/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityPresenter.kt
Outdated
Show resolved
Hide resolved
...n/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityPresenter.kt
Outdated
Show resolved
Hide resolved
…_profile_screen_inside_a_fragment
@yash10019coder I can't be sure what's wrong without actually debugging it, but in principle I actually suggest trying to implement this without using the backstack, if possible. I generally recommend against using the fragment backstack because it can be difficult to get correct, and I've yet to run into a situation where it was actually needed. Generally, the alternatives end up being more predictable and easy to understand, too. Can you provide a bit more context into what the exact user flow is that you're trying to model? |
… build.gradle(:app).
…_profile_screen_inside_a_fragment
…de_a_fragment' of github.com:yash10019coder/oppia-android into hi-fi-profile-tablet-Implement_edit_profile_screen_inside_a_fragment
…or_controls_activity.xml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @BenHenning mentioned all the comments PTAL thanks.
...edTest/java/org/oppia/android/app/administratorcontrols/AdministratorControlsFragmentTest.kt
Outdated
Show resolved
Hide resolved
…blet-Implement_edit_profile_screen_inside_a_fragment
Hi @yash10019coder, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Reopened since this was closed due to my late review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yash10019coder. Sorry for the late review. Overall PR is LGTM, just had a few comments--PTAL.
app/src/main/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivity.kt
Outdated
Show resolved
Hide resolved
...n/java/org/oppia/android/app/administratorcontrols/AdministratorControlsFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks.
…blet-Implement_edit_profile_screen_inside_a_fragment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @BenHenning Made the requested changes PTAL thanks.
app/src/main/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivity.kt
Outdated
Show resolved
Hide resolved
...n/java/org/oppia/android/app/administratorcontrols/AdministratorControlsFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yash10019coder! This LGTM.
Explanation
Fixes #4028
Fixes #3242
Fixes #4007
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then:
Mobile Screenshots
Tablet Screenshots
UI Workflow Mobile
oppia-phone-2022-02-08_23.08.12.mp4
UI Workflow Tablet
oppia-tablet-2022-02-08_23.04.40.mp4
AdministratorControlsActivityTest and AdministratorControlsFragmentTest