Closed nostrbuddha closed 5 days ago
@nostrbuddha I'm reviewing this today. For now just added one commit to resolve conflicts with main branch so CI can run and I can download and test, please review when you have a moment.
CI not building, I'll have a look to see if an easy fix to do a full review, otherwise I'll just focus on pure code review for now till you have a look
* What went wrong:
Execution failed for task ':shared:presentation:compileDebugUnitTestKotlinAndroid'.
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.GradleCompilerRunnerWithWorkers$GradleKotlinCompilerWorkAction
> Compilation error. See log for more details
@nostrbuddha Just did a proper rebase of the branch and it works like charm :D
CI still failing though
AAPT2 aapt2-8.5.0-11315950-linux Daemon #0: shutdown
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':shared:presentation:compileDebugUnitTestKotlinAndroid'.
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.GradleCompilerRunnerWithWorkers$GradleKotlinCompilerWorkAction
> Compilation error. See log for more details
UI not passing because androidNode cannot build in this branch, more details coming in the first full review
@nostrbuddha this is looking really good, I think the next step would be to rebase your fork branch against bisq-mobile main and do the necessary adaptions to use DI as discussed on matrix.
After that, I'll update my local copy of your branch, test it again in the 3 apps and we'll tweak any small details missing and merge it!
If you have doubts on how to do that or any other questions please reach out on here or directly on chat!
@nostrbuddha I've installed and tested on the 3 apps and it works amazing even though the build failed in the CI (linux)
I don't have access to a linux machine now, but will do later to verify what's going on there.
In the meantime, runtime-wise this is grea - only issues I could find are very small nuances:
Not a deal breaker for this merge, we can report that in individual issues and tackle them with full force later. So gonna review the code now as requested and put my comments and we can do a final review pass to merge in by the weekend hopefully 💪 great job buddha
@nostrbuddha I've committed small nits and the connection between main presenter and the rest of them.
Result is clearly visible now, if you put the app in background and come back it updates the btc price (from Loading to 0 at the moment)
now every presenter needs the main present to be passed as first param, very easy in Koin module context just use get() :)
Gonna test it in iOS and do any changes if necessary - merge is imminent... :D
@nostrbuddha Also added some swift code for a better onResume() behaviour omn iOS
For the main screen we probably want to call the corresponding onResume() each time the user change tabs? or maybe an specific method - something we need to think about and probably we'll figure out on the next bunch of UI - well done Buddha!
This PR has the following items done:
All screens goes into /screens All reusable components in /components and such.
I am specifically looking for review on how MVP pattern is applied.
Based on feedback, I can keep pushing into this branch. Once it's all good to go, it can then be merged with main.
Thanks.