Skip to content
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

Migrated Bookmarks Package From Butterknife to ViewBinding #5594

Merged
merged 3 commits into from
Mar 17, 2024

Conversation

shashankiitbhu
Copy link
Contributor

Description (required)

Related to #4664

What changes did you make and why?

Tests performed (required)

Tested {build variant, e.g. ProdDebug} on {name of device or emulator} with API level {API level}.

Screenshots (for UI changes only)

Need help? See https://support.google.com/android/answer/9075928


Note: Please ensure that you have read CONTRIBUTING.md if this is your first pull request.

ButterKnife.bind(this, v);
binding = FragmentBookmarksPicturesBinding.inflate(inflater, container, false);
View v = binding.getRoot();
gridView = binding.bookmarkedPicturesList;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, this does the work but doesnt give you the optimality.
This will make you have import android.widget.GridView; which is unnecessary as this will make uneccessary import and we will lose the advantage of viewbinding here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer PR #5598

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I was using this for tests to work, In your original PR you were not handling tests (checks did not run on your PR), but I have found a better way to handle tests now.

@neeldoshii
Copy link
Contributor

Hi @shashankiitbhu,
I have left few reviews.

Review by Student.

@shashankiitbhu
Copy link
Contributor Author

shashankiitbhu commented Mar 3, 2024

@neeldoshii will look at them soon, thanks

@neeldoshii
Copy link
Contributor

Thanks for the contribution.
One more minor changes, add this at the end in each package @shashankiitbhu. This will help you to provide null safety, memory leaks. Rest looks good from my side. After this approved from my side.

@Override
public void onDestroyView() {
    super.onDestroyView();
    binding = null; // Nullify the binding
}

Review by Student.

@psh psh merged commit 161e2ed into commons-app:main Mar 17, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants