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
Automatically show keyboard after learn more bottom sheet is dismissed (PSG-1105) #7790
Automatically show keyboard after learn more bottom sheet is dismissed (PSG-1105) #7790
Conversation
This reverts commit f0dc6e4.
SessionLearnMoreBottomSheet.show(childFragmentManager, args) | ||
SessionLearnMoreBottomSheet | ||
.show(childFragmentManager, args) | ||
.apply { |
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.
I think in general in the project, we try to avoid using apply
to avoid bugs on the scope of objects. Maybe it will be better to write it this way:
SessionLearnMoreBottomSheet
.show(childFragmentManager, args)
.onDismiss = { showKeyboard() }
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.
Done.
views.renameSessionEditText.doOnTextChanged { text, _, _, _ -> | ||
viewModel.handle(RenameSessionAction.EditLocally(text.toString())) | ||
} | ||
} | ||
|
||
private fun showKeyboard() { |
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.
private fun showKeyboard() { | |
private fun showKeyboard() { | |
val focusChangeListener = object : ViewTreeObserver.OnWindowFocusChangeListener { | |
override fun onWindowFocusChanged(hasFocus: Boolean) { | |
if (hasFocus) { | |
views.renameSessionEditText.showKeyboard(andRequestFocus = true) | |
} | |
views.renameSessionEditText.viewTreeObserver.removeOnWindowFocusChangeListener(this) | |
} | |
} | |
views.renameSessionEditText.viewTreeObserver.addOnWindowFocusChangeListener(focusChangeListener) | |
} |
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.
I think we should remove the listener once done (see my suggestion above). Otherwise it will keep adding a listener each time and every listeners will be called.
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.
Right, done.
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 for the fix! I think we need to remove listeners on the EditText
after adding one. See my related comment.
SonarCloud Quality Gate failed. |
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 for the updates! LGTM.
Type of change
Content
For a better UX, if an EditText is focused and the keyboard is already shown after showing and hiding a bottom sheet then the expected behavior is to show the keyboard by default.
Motivation and context
UX review remarks.
Screenshots / GIFs
show_keyboard_after_bottomsheet.mp4
Tests
Tested devices
Checklist