-
Notifications
You must be signed in to change notification settings - Fork 4
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
UI branch - Draft PR #36
Conversation
@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
|
@nostrbuddha Just did a proper rebase of the branch and it works like charm :D |
CI still failing though
|
UI not passing because androidNode cannot build in this branch, more details coming in the first full review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nostrbuddha - I'm stoked with your first UI implementation contribution - great worked!
I just did a first pass on code review and execution and left comments for your to review / discuss. Alongside with that I would like to point out the followings in an attempt to help you organise and make this Pair review work a breeze:
- As a general comment, your PR is a great contribution on the right direction. You've done a great job integrating your UI work to the base proposed architecture done with a few lines of examples, you've leveraged KMP compose libs first whenever possible, used coroutines when appropiate, and added well structured code. Also you've done more than expected provided foundation for i18n and some networking setup.
- On that note, CI not building -> this is because this branch breaks the
androidNode
app. I left you a comment on why this is a happening and how to fix it, its related to ktor dependency -> I suggest we work on this one first - I ran the
xClients
on a real iPhone 10 and android emulators - What a great feeling :D . Something to point out, in my iPhone 10 things are not looking as sharp as in android, with some texts being double lined for example. I think is because of my test device accessibility settings increasing text size, we'll discuss on this on matrix if you agree. - This is a first pass and there is quite a bit to work on so I'll leave it here to discuss/work on the stuff and then we'll do a second pass if you agree.
Overal, great job!!! This is very exciting.
...d/domain/src/commonMain/kotlin/network/bisq/mobile/domain/data/repository/PriceRepository.kt
Outdated
Show resolved
Hide resolved
bisqapps/shared/presentation/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/App.kt
Show resolved
Hide resolved
.../presentation/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/screens/URLScreen.kt
Outdated
Show resolved
Hide resolved
...on/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/navigation/graph/TabNavGraph.kt
Outdated
Show resolved
Hide resolved
...ntation/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/screens/SplashPresenter.kt
Outdated
Show resolved
Hide resolved
...n/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/navigation/graph/RootNavGraph.kt
Outdated
Show resolved
Hide resolved
@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! |
…coil3 lib dependency
- Script to convert translation keys, values from bisq2 app to mobile app lyricist structure - Translation strings in onboarding screens - Key, Values updated for all strings used in bisq2's application module.
7542a3a
to
85c73ca
Compare
- Made use of Koin DI in SplashScreen, OnboardingScreen, GettingStartedScreen - /screens to /uicases/*
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job @nostrbuddha we are very close now.
The main changes requested in terms of code are
- some missing usages of DI I could spot
- using the new shared repository base classes, and structure to always get / save data to the models in them (otherwise we will loose updates from the network)
- and some minor other changes
I hope you don't find this very tedious, but I consider your PR fundational to the bisq apps as it sets basic "how to's" that will be reused across the UI development of it for the rest of the MVP. Therefore, very important to have it merged right.
As usual, feel free to challenge and question anything I'm reviewing here.
When we finish with this last changes, it's just a matter of doing a final test and making sure it passes the CI (I think it fails because some unintended gradle and pods changes you've commited , once you delete that it will pass :) )
Cheers!!!
...main/src/commonMain/kotlin/network/bisq/mobile/domain/data/repository/BisqStatsRepository.kt
Outdated
Show resolved
Hide resolved
...domain/src/commonMain/kotlin/network/bisq/mobile/domain/data/repository/NetworkRepository.kt
Outdated
Show resolved
Hide resolved
...src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/exchange/ExchangeScreen.kt
Outdated
Show resolved
Hide resolved
...commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/startup/CreateProfileScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/startup/OnBoardingScreen.kt
Show resolved
Hide resolved
...rc/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/startup/OnBoardingScreen.kt
Show resolved
Hide resolved
...ation/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/startup/URLScreen.kt
Outdated
Show resolved
Hide resolved
- Better data models and repositories in 'domain' - Better DI usage with screens and presenters, by moving navControllers into DI (But having some issue now, which stops tab based nav from working. RootNavGraph, TabNavGraph probably need some refactor) - Few other minor feedback corrections
… fields public already generates those
…be reviewed by buddha)
…xes and improvements. This fixes the build issue on this branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nostrbuddha we can see the finish line now - please run git pull --rebase
on your local branch to grab the updates and then ./gradlew clean build
(this one might take a while cause we needed to upgrade gradlew to version 8.11) .
Once you are ready have a look at the last unresolved comments and we are set - go go go! :D
...presentation/src/commonMain/kotlin/network/bisq/mobile/presentation/di/PresentationModule.kt
Outdated
Show resolved
Hide resolved
.../presentation/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/navigation/README.md
Show resolved
Hide resolved
.../presentation/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/navigation/Routes.kt
Show resolved
Hide resolved
...ps/shared/domain/src/commonMain/kotlin/network/bisq/mobile/domain/data/model/BtcFiatPrice.kt
Outdated
Show resolved
Hide resolved
...ps/shared/domain/src/commonMain/kotlin/network/bisq/mobile/domain/data/model/NetworkModel.kt
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,14 @@ | |||
package network.bisq.mobile.domain.data.model | |||
|
|||
// TODO: Is it okay for the models to be mutable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question refers to the open
..? Unless there is a really strong inheritance relationship, its better to use composition.
…senters + small nits
@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.