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

fix : migrating fragments from butterknife to viewbinding #2146

Conversation

PratyushSingh07
Copy link
Collaborator

Fixes #2142

This PR lists the fragments that have been migrated to viewbinding :

  • LocationFragment
  • TransferProcessFragment
  • RecentTransactionsFragment
  • QrCodeReaderFragment
  • ThirdPartyTransferFragment
  • QrCodeImportFragment
  • QrCodeDisplayFragment

@PratyushSingh07 PratyushSingh07 changed the title fix #2142 : migrating fragments from butterknife to viewbinding fix : migrating fragments from butterknife to viewbinding May 22, 2023
@PratyushSingh07
Copy link
Collaborator Author

@gururani-abhishek do review this whenever you are free

@jawidMuhammadi jawidMuhammadi added this to In progress in GSoC 2023 Work Progress via automation May 22, 2023
@PratyushSingh07 PratyushSingh07 changed the base branch from development to migrate_to_view_binding May 23, 2023 03:21
Copy link
Contributor

@gururani-abhishek gururani-abhishek left a comment

Choose a reason for hiding this comment

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

  1. In LocationsFragment.kt, line 8,9 please remove unused Butterknife imports.

  2. In ThirdPartyTransferFragment.kt, RecentTransactionsFragment.kt, please double check if errorLayoutBinding is required. I think we can use binding to get a reference of the included error_layout.

  3. At some places I've remarked to do view interactions in onViewCreated(), according to android documentation for onCreateView() : "It is recommended to only inflate the layout in this method and move logic that operates on the returned View to onViewCreated()"
    Here's the documentation link for better reference.

  4. In RecentTransactionsFragment.kt bang-bang operator is used with activity and context to assert them as non-null. In all these places(lines : 99,117, 178, 186) please reformat the code to requireActivity() and requireContext().

GSoC 2023 Work Progress automation moved this from In progress to Review in progress May 24, 2023
recentTransactionsPresenter?.attachView(this)
sweetUIErrorHandler = SweetUIErrorHandler(activity, rootView)
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using "binding.root" twice, it'll be a great for the purpose of clean coding if we create a val rootView = binding.root, and use it wherever Root View is required. Just a suggestion, totally your call.

@PratyushSingh07
Copy link
Collaborator Author

hey @jawidMuhammadi do review this whenever you are free

Copy link
Contributor

@jawidMuhammadi jawidMuhammadi left a comment

Choose a reason for hiding this comment

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

LGTM! :)

GSoC 2023 Work Progress automation moved this from Review in progress to Reviewer approved May 27, 2023
Copy link
Contributor

@gururani-abhishek gururani-abhishek left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jawidMuhammadi jawidMuhammadi merged commit 262160d into openMF:migrate_to_view_binding May 29, 2023
3 checks passed
GSoC 2023 Work Progress automation moved this from Reviewer approved to Done May 29, 2023
@PratyushSingh07 PratyushSingh07 deleted the fragments-viewbinding branch June 7, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Investigate Butterknife migration and create a report
3 participants