Migrate CollapseFullScreenDialogFragment.java to ViewBinding
#20975
Conversation
neeldoshii
force-pushed
the
Migrate-`CollapseFullScreenDialogFragment`-to-ViewBinding
branch
from
3e755fc
to
20d74b8
Compare
| @@ -56,6 +56,7 @@ public class CollapseFullScreenDialogFragment extends DialogFragment { | |||
| private static final String ARG_HIDE_ACTIVITY_BAR = "ARG_HIDE_ACTIVITY_BAR"; | |||
| private static final String ARG_TITLE = "ARG_TITLE"; | |||
| private static final int ID_ACTION = 1; | |||
| private CollapseFullScreenDialogFragmentBinding mBinding; | |||
There was a problem hiding this comment.
Reviewer Note (@ParaskP7): Should we always have refference of binding locally?.
also do you think I should making binding null on dismiss?
@Override
public void dismiss() {
if (mHideActivityBar) {
showActivityBar();
}
try {
getParentFragmentManager().popBackStackImmediate();
} catch (IllegalStateException e) {
e.printStackTrace();
}
}
There was a problem hiding this comment.
👋 @neeldoshii !
Should we always have refference of binding locally?
Since the mBinding is being used in both, the onCreate(...) and onCreateView(...) you can't avoid but using it as you did, via a shared field. Thus, the answer to your question is that "it depends".
also do you think I should making binding null on dismiss?
I actually suggest doing that within the onDestroyView(), just like it is recommended by the docs, which is to use the onDestroy() lifecycle method(s) and remove any reference to the mBinding fields there.
|
👋 @neeldoshii and thanks so much for submitting yet another PR! Let's first manage to merge the other 2 open PRs that we have opened so far, that is, before proceeding to reviewing or opening any more PRs like that. I would love if you take all the lessons learned so far and start applying them to subsequent PRs, that, without us having to re-discuss on topics that we have already touched before. How does this plan sound to you? 😊 PS: I don't want to stop you from being productive, nor to bring your morale down, I just want us to have an efficient process moving forward, 1 step at a time, 1 PR at a time. Then, when we'll start flying high, we could do more PRs in parallel. I hope that doesn't displease you. 🙏 |
…nDialogFragment`-to-ViewBinding
|
There was a problem hiding this comment.
👋 @neeldoshii !
Thank you so much for your contribution to JP/WPAndroid with this PR! 🥇
I have reviewed but NOT tested this PR, neither triggered CI on it just yet, good job except with the testing part! 🌟
You will notice my blocker comment (🚫) below. Please make every effort to build and test any change you make, no matter how small. Also, please make an effort to check your code against our static analysis tools, like CheckStyle, Detekt and Lint, that is, before pushing it to remote. Not doing so gives me little confidence on the code that is about to be reviewed and doesn't make it easy for us to merge your solution as quick as possible.
| @Override | ||
| public void onDestroy() { | ||
| super.onDestroy(); | ||
| mBinding = null |
There was a problem hiding this comment.
Blocker (🚫): ; is expected here, this is a Java file. Don't forget to build and test your code before pushing. 🤷
|
Hi @ParaskP7 👋
Not sure how did this happened 😅 when I tested locally it wasn't there as well as CI passed strange! My apologies for it 😔. I have made the appropriate changes. |
|
👋 @neeldoshii !
🤷
What wasn't there? 🤔
CI didn't pass, I need to trigger CI manually after reviewing your changes, which I did, and then noticed the problem, at which point I didn't trigger CI.
I don't see any updates, are you sure you pushed the change(s)? 🤔 |
Correct! Pushed now. |
|
Awesome, thanks @neeldoshii , I'll take another look at it today! 👍 |
There was a problem hiding this comment.
👋 @neeldoshii !
Thank you so much for your contribution to JP/WPAndroid with this PR! 🥇
I have reviewed and tested this PR as per the instructions, along with triggering CI on it via a draft PR, which I just pushed on the main repo, everything works as expected, good job! 🌟
|
🎉 @neeldoshii , another contribution in, thanks so much for all your work! 🥇 |
Description
This pull request migrates the
CollapseFullScreenDialogFragmentto use ViewBinding, improving type safety and eliminating the need for findViewById calls.Fixes/Related
findViewByIdwithViewBinding#19180Screenshots/ Video
// TODO
Steps to reproduce
CollapseFullScreenDialogFragmentwill be reproduced 🥇PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):