ViewBinding Migration #629
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…homeAdapter to viewBinding) and GsrBuildingAdapter viewBinding implementation
…l crash out worthy debug session)
…why does diningsettings use views from laundry lmao)
…oadingpanel fixes
…o-refresh implmentation in onResume so we don't need to manually hide the no internet view on PottruckFragment
… the switches as the fragment has no access to the layout anymore (and also how do we even access PennCourseAlerts?)
baronhsieh2005
requested review from
meiron03,
Akula112233,
trinif,
Divak2004,
vavali08 and
joemacd
meiron03
reviewed
Oct 6, 2024
| @@ -164,39 +143,32 @@ class HomeAdapter( | |||
| } | |||
|
|
|||
| else -> { | |||
There was a problem hiding this comment.
Why don't the DiningCell, CalendarCell, and LaundryCell get their own ViewHolders? Although it's less code since they share the same views (for now), I don't think it's a good idea to blend all of these into one. Ultimately, they are different things and may have different needs in the future.
Maybe better is to make a base holder (e.g., HomeBaseHolder) that all of the other ViewHolders in the home page inherent from. Then, in this case you can basically have a "HomeDiningHolder" that just inherits from HomeBaseHolder without any fields of its own. IMO things are more natural this way.
|
Everything else seems good to me though. Great job, this is huge! |
… binding instead of findviewbyid
…ls can have their own viewholders
meiron03
reviewed
Oct 7, 2024
PennMobile/src/main/java/com/pennapps/labs/pennmobile/viewholders/HomeBaseHolder.kt
Show resolved
Hide resolved
|
Yessirrrr |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
Access
toolbarfrommActivity.findViewByIdas it is a component of theinclude_mainlayoutSince all views that require
loadingPanelalready has access to the reuseable layout throughinclude, I mostly just referenced it throughview.findViewByIdor through the bindingAs
no_resultsis also a reusable layout, I gave all views that require the layout access throughincludeFor most adapters, I rewrote them such that their viewholders would take in bindings so they can access and reference the view objects directly through the binding and bind the views inside the viewholders OR functions in the adapter can take in these viewholders as arguments and bind the view (The case of
HomeAdapter)Also additionally added viewbinding for some fragments that don't use it.
Changed the Parcelize package used in
SavedStateto the non-deprecated version (enabled by the kotlin-parcelize gradle plugin)Removed abundant "not connected to internet" banner/toolbar in DiningHolderFragment since
DiningFragmentandDiningInsightsFragmentalready handle these logics themselves (and there would be two toolbars that pops up when there is no internet)Some other lifecycle tweaks (so that we won't do any view work during
onCreateand crash the app)Now, after this, we can start upgrading basically everything without too much issues. I have successfully implemented compose currently on a branch with kotlin ver. 1.8.0, but I think it's best to review the migration and we can finalize all the upgrades/implementation in a meeting in person