[MBL-789] ProjectPageViewModel.kt to Jetpack ViewModel and RxJava2.2 #1890
Conversation
This file contains hidden or 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
Arkariang
reviewed
Oct 30, 2023
app/src/test/java/com/kickstarter/viewmodels/ProjectPageViewModelTest.kt
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1890 +/- ##
============================================
- Coverage 73.93% 73.89% -0.04%
- Complexity 1945 1950 +5
============================================
Files 339 339
Lines 19486 19501 +15
Branches 2698 2707 +9
============================================
+ Hits 14407 14411 +4
- Misses 3526 3528 +2
- Partials 1553 1562 +9
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Arkariang
reviewed
Nov 1, 2023
Arkariang
reviewed
Nov 1, 2023
|
|
||
| binding.backIcon.setOnClickListener { | ||
| (this as BaseActivity<*>).back() | ||
| onBackPressedDispatcher.onBackPressed() |
Arkariang
reviewed
Nov 1, 2023
| } | ||
|
|
||
| public override fun onPause() { | ||
| super.onPause() | ||
| binding.mediaHeader.inputs.releasePlayer() | ||
| } | ||
|
|
||
| override fun back() { | ||
| override fun onBackPressed() { |
There was a problem hiding this comment.
Can be removed, is deprecated and the back navigation can be handled by adding:
this.onBackPressedDispatcher.addCallback {
finishWithAnimation()
}
Arkariang
reviewed
Nov 1, 2023
|
|
||
| binding.backIcon.setOnClickListener { | ||
| (this as BaseActivity<*>).back() | ||
| onBackPressedDispatcher.onBackPressed() | ||
| finishWithAnimation() |
There was a problem hiding this comment.
This finishWithAnimation() can be removed, on the previous line you are calling any of the callbacks registered for the back navigation.
Asumming of course that the callback is defined like:
this.onBackPressedDispatcher.addCallback {
finishWithAnimation()
}
Arkariang
reviewed
Nov 1, 2023
| */ | ||
| fun refTag(intent: Intent): Observable<RefTag?> { | ||
| return Observable.just(intent.getParcelableExtra(IntentKey.REF_TAG)) | ||
| fun refTag(intent: Intent): io.reactivex.Observable<KsOptional<RefTag?>> { |
Arkariang
reviewed
Nov 1, 2023
app/src/main/java/com/kickstarter/viewmodels/usecases/SendThirdPartyEventUseCase.kt
Outdated
Show resolved
Hide resolved
Arkariang
reviewed
Nov 1, 2023
|
|
||
| intent() |
There was a problem hiding this comment.
just an fyi, your approach works perfectly, but in order reduce amount of code changes, take a look for example at MessagesActivity
The intent can be passed to the viewmodelProvider factory, and then have a private method
private fun intent() = intent?.let { Observable.just(it) } ?: Observable.empty()
in that way you don't need to create the new input fun configureWith(intent: Intent)
Arkariang
reviewed
Nov 1, 2023
app/src/main/java/com/kickstarter/viewmodels/projectpage/ProjectPageViewModel.kt
Outdated
Show resolved
Hide resolved
…s test is triggering `saveProject:517` + `toggleProjectSave:475` resulting on the 3 emissions
b79c7be to
b9b465e
Compare
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.
📲 What
Migrate project page viewmodel to rxjava 2.2 and jetpacks viewmodel
🤔 Why
Technical Details
👀 See
No user facing changes
📋 QA
Test project page loading correctly from all access points (discovery, search, profile, etc), all tabs, watching/unwatching project while logged in and out, and back a project. This will require additional QA time.
Story 📖
MBL-789: ProjectPageViewModel.kt to Jetpack ViewModel and RxJava2.2