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 FeedbackBottomSheet rotation bug #699
Conversation
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.
Minor details to address before 🚢
|
||
@Override | ||
public void onDestroyView() { | ||
// Prevent listener leak |
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.
What about extracting this block of code into a private
method and give it a name based on the comment? This way onDestroyView()
will be easier to read and understand and comment will become superfluous so it won't be necessary.
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.
We should keep public
methods as clean as possible so they read like pseudocode (i.e. no conditionals, for
s, etc.).
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.
👍
// Prevent listener leak | ||
feedbackBottomSheetListener = null; | ||
|
||
// Maintains visibility on rotation |
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.
Same here ☝️
if (dialog != null && getRetainInstance()) { | ||
dialog.setDismissMessage(null); | ||
} | ||
// Cancel the animation |
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.
Same here ☝️
@Override | ||
protected void onAttachedToWindow() { | ||
super.onAttachedToWindow(); | ||
// Attach the listener to the FeedbackBottomSheet if it is showing |
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.
What about extracting this block of code into a private
method giving it a name based on the comment and then remove it (comment won't be necessary anymore)?
@Guardiola31337 thanks, should be good to go 🍍 |
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.
🚀
2097d38
to
66e158d
Compare
Closes #693