-
Notifications
You must be signed in to change notification settings - Fork 521
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 #1438 & #1761: Switch Profile Dialog lost in orientation change #1757
Fix #1438 & #1761: Switch Profile Dialog lost in orientation change #1757
Conversation
/** [DialogFragment] that gives option to either cancel or exit current profile */ | ||
class NavigationDrawerSwitchProfileDialogFragment : DialogFragment() { | ||
|
||
private lateinit var menu: Menu |
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.
One suggestion I am looking for here:
Another approach is where we can have an interface and rather than passing menu
and drawerLayout
object we can implement an interface in NavigationDrawerFragment
and use it from there.
The only thing we have to achieve with these objects is
menu.getItem(
NavigationDrawerItem.HOME.ordinal
).isChecked =
true
drawerLayout.closeDrawers()
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.
This approach does sound correct. Please have a look at top level comment.
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.
This similar implementation is used in HomeActivity
also, so I suggest applying this fragment there too.
Also, please update/rename NavigationDrawerSwitchProfileDialogFragment
so that its not just linked to NavigationDrawer.
Something like ExitProfileDialogFragment
Also, update test cases accordingly.
app/src/main/java/org/oppia/app/drawer/NavigationDrawerFragmentPresenter.kt
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.
Few nits/questions before final approval.
app/src/main/java/org/oppia/app/drawer/ExitProfileDialogFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/drawer/NavigationDrawerFragmentPresenter.kt
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
Explanation
Fix #1438 #1761
How to test
How to review
Check if the added test case works
Checklist