UI iteration 2 #73

nostrbuddha · 2024-11-25T04:56:19Z

MyTrades UI (3rd tab) with Presenter, model, repository and mock data
Market screen: CurrencyListing screen with Presenter, model, repository and mock data
Market screen: OfferListing screen

These are basic UI and miss functionality like search, filters, switch between buy / sell. But I guess it's good enough to pull real data from bisq libs and populate in view for testing.

...n/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/trades/MyTradesScreen.kt

rodvar

outstanding work buddha 👏

Besides my code review comments, here is my functional review

This PR is the perfect opportunity to implement back button navigation. At the moment, once you get to the Offers screen the only way to get out is by swiping left (which is great), but we need also the standard visual left arrow on the top left (next to the title) as it has become industry std in both iOS and Android these days. If you think this is a big deal please let me know and we can do it in a separate feature.
Search bar: I can see that is just mocked at the moment, do we have a separate issue to work this one out? I'll refrain from commenting on that one for now (unless you ask me to do it now)

That's it for this first pass, let me know if you have any questions! 💪

rodvar · 2024-11-25T08:37:54Z

bisqapps/shared/domain/src/commonMain/kotlin/network/bisq/mobile/domain/data/model/MyTrades.kt

@@ -0,0 +1,26 @@
+package network.bisq.mobile.domain.data.model
+
+// TODO: Update later


can we please be more specific with the TODO? what can of update are we referring to here?

My bad. Will update the comment!

I meant to refer Bisq2 for the data model structure of Offers and replicate the same here

...ain/src/commonMain/kotlin/network/bisq/mobile/domain/data/repository/CurrenciesRepository.kt

rodvar · 2024-11-25T08:49:09Z

.../src/commonMain/kotlin/network/bisq/mobile/presentation/ui/components/CurrencyProfileCard.kt

+            //DynamicImage("drawable/${currency.flagImage}", contentDescription = null)
+            println(currency.flagImage)
+            DynamicImage("drawable/${currency.flagImage}", contentDescription = null)
+//            AsyncImage(


please remove dead code

Yes. Will make sure this never happens again!

haha no big deal but whenever we are sure let's keep it clean :)

rodvar · 2024-11-25T08:52:38Z

.../src/commonMain/kotlin/network/bisq/mobile/presentation/ui/components/layout/StaticLayout.kt

+
+@Composable
+fun BisqStaticLayout(
+    innerPadding: PaddingValues = PaddingValues(top = 48.dp, bottom = 12.dp, start = 12.dp, end = 12.dp),


not sure if its related to this code, but there is a ui bug for "small phones" like pixel 6a where the top title bar overlaps part of the screen you are looking at . Also the bar is not stretching to cover all the width of the screen as it should.

Yeah. I am not happy with this layout components. Have a few internal TODO items.
And yes, I need to test in small screens as well.

Shall I create a separate issue for the layout components/small screen testing and track this issue over there?

@nostrbuddha yes please - let's create a separate issue so we don't loose track of this and eventually tackle it 💪

Created one @ #79
Pls assign this to me

rodvar · 2024-11-25T08:54:06Z

...src/commonMain/kotlin/network/bisq/mobile/presentation/ui/components/molecules/BisqDialog.kt

+import network.bisq.mobile.presentation.ui.theme.BisqTheme
+
+@Composable
+fun BisqDialog(


the "Yes, please" is double lining on a pixel 6a. also buttons shapes are not the same size and not aligned (not good for UX)

oh okay. I ll create pixel 6a emulator and start testing in it.

I just happen to have that testing device, but the comment is more of the UI rules applying to that component - decision buttons in general is good to have them aligned and same size to communicate "hey you have to choose from this 2 options" kinda thing :)

Yes. Will cover this in #79 as well

rodvar · 2024-11-25T08:59:10Z

...ommonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/offers/CurrencyListPresenter.kt

+    private fun refresh() {
+        CoroutineScope(BackgroundDispatcher).launch {
+            try {
+                delay(1000) // TODO: To simulate loading. Yet to be handled


good simulation

Might be a good candidate to incorporate a subtle spinner with bisq colors in the middle when we are waiting for I/O response? The idea is that the view depends on a "showLoading" presenter flag that gets turned on before the delay and turned off after. This implementation is important to get right as it will be copied every time we need to wait for I/O - please let me know if you have questions

The markets (currencies called here) are static and created on node at startup and at client we request it but we can persist (or even hard code) it as its never changing.

An open question is thought if we want to show all markets (about 150) or just show the main currencies or those where there are offers, and use the search to allow the user to add more markets (once selected from search it stays in the list). But we can delegate that for later fine tuning.

General rule in mobile is don't show anything that the user cannot interact with. If there are no offers it should be hidden, but as you said let's leave this for fine tuning! ;)

I was wrong its not 150 markets as we filter for those where we have price, i guess about 30- 40 then... added the filter in my new branch. But still agree we might find a better UX to not show empty markets in a fine tune phase.

So there is filter option in the CurrencyListing (yet to be implemented).

In Bisq2 desktop, by default it shows all currencies.
But in mobile, we can set the showmarkets filter to only with offers by default.

and right now, I will hardcode all 150+ currnecies that are shown in Bisq2 desktop app.

I would suggest to leave the loading topic for later. For the markets / currency list I doubt it will be needed (at node its instant, at client we can hard code data as its static data).
Of other screens like offer list, the list gets automatically updated with data, so data stream in. That is also the case in Desktop where at first start it takes a bit until the offers arrive and then fill up the list as they arrive.

But I think different user cases will have diff. context here, which will be easier to analyze once the core functionality is implemented.

So there is filter option

I would keep that also for later, it at all (IMO we should try to keep mobile to the most minimal feature set as possible).

I will hardcode all 150+ currnecies that are shown in Bisq2 desktop app.

No need. I have it already connected with real data.

@nostrbuddha your comment on the scenarios can go straight into the description of a new "Loading Spinners" UI issue ;)

Created at #80

rodvar · 2024-11-25T09:01:01Z

...src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/offers/OffersListScreen.kt

+                    if (openDialog.value) {
+                        BisqDialog {
+                            ConfirmationDialog(
+                                message = "Do you want to take this trade?",


Are we dropping the dependency on the translator or we forgot to add it here?

I'm ok if we hardcode in english for the MVP just want to make sure is a conscious decision.

Do we need the dialogue? In Bisq Easy we dont have it but open the take offer screens (select the available options or get review screen if nothing to select). Though we have plenty of warnings with the reputation based amount limits. That will be painful to replicate ;-(.

great question. I think we should just go to the trade details and the user can decide if they want to take it there?
For now we can just log something on click to indicate the todo

good that he developed a dialog though, I'm sure it will be handy somewhere :) ("Confirm Trade paid? Y/N e.g.")

Are we dropping the dependency on the translator or we forgot to add it here?

I'm ok if we hardcode in english for the MVP just want to make sure is a conscious decision.

My bad!! Missed it. I will add the translation strings everywhere

Yes. Will remove the Dialog from here

nostrbuddha · 2024-11-26T04:49:52Z

outstanding work buddha 👏

Besides my code review comments, here is my functional review

1. This PR is the perfect opportunity to implement back button navigation. At the moment, once you get to the Offers screen the only way to get out is by swiping left (which is great), but we need also the standard visual left arrow on the top left (next to the title) as it has become industry std in both iOS and Android these days. If you think this is a big deal please let me know and we can do it in a separate feature.

2. Search bar: I can see that is just mocked at the moment, do we have a separate issue to work this one out? I'll refrain from commenting on that one for now (unless you ask me to do it now)

That's it for this first pass, let me know if you have any questions! 💪

Yeah. Missed that one. Guess Henrick already implemented the back button. And we discussed (in matrix) a way of improving and generalising it inside the TopBar. I will do that now.
I will implement search functionality as well in this PR now.

HenrikJannsen · 2024-11-26T05:46:27Z

2. I will implement search functionality as well in this PR now.

You can leave that to do after the domain integration as it will likely be different than. I have already a filtered and sorted list and it will be easy to add that predicate.

nostrbuddha · 2024-11-26T07:02:08Z

@rodvar Made some updates now. Mostly minor:

dead code removal, better comments, translation keys/values

Others:

Adding all currencies are done by Henrik in his commits
For Search, I will work after his merge, as suggested by Henrik
For small screen layout issues, I will create another issue and solve it.
Back button generalisation - I will do once Henrik's code is merged
Loader - Not for now.

rodvar · 2024-11-26T10:03:05Z

@rodvar Made some updates now. Mostly minor:

dead code removal, better comments, translation keys/values

Others:

Adding all currencies are done by Henrik in his commits

For Search, I will work after his merge, as suggested by Henrik

For small screen layout issues, I will create another issue and solve it.

Back button generalisation - I will do once Henrik's code is merged

Loader - Not for now.

excellenty buddha, I'll review and try to get this merge as is. Please create those issues so we don't look this track (or let me know if you want me to create them for you).

Cheers!

- Dialog component - Exchange screen improvements - Offerbook UI - Basic

…and OfferListScreen

…issed staging the files)

rodvar

cool buddha, just gonna fix the comment I've added now and merge it.

We need to create some GH issues to not loose track of the things we have decided to work on later in the context of this PR.

I'll add one more here:

on iOS: standard left-to-right swipe is not going backwards on the view stack (in android back swipe which is the opposite direction works fine)

I'll let you decide if you prefer to create the issues yourself, happy to help you if you want to just keep the coding momentum - just please let me know 🙏

rodvar · 2024-11-26T10:43:06Z

...n/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/trades/MyTradesScreen.kt

+
+    val myTrades: List<BisqOffer> = presenter.myTrades.collectAsState().value
+
+    LaunchedEffect(Unit) {


Let's avoid LaunchedEffect in favour of our new composable RememberPresenterLifecycle -> I'll update this PR so we can merge it now

Similar to creating custom hooks in React - love it!
Thanks for showing this approach. 😊

rodvar · 2024-11-26T10:44:58Z

...n/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/trades/MyTradesScreen.kt

+        presenter.onViewAttached()
+    }
+
+    if (myTrades.isEmpty()) {


good reactive ui programming

…cle which handle both attach and unattach

HenrikJannsen suggested changes Nov 25, 2024

View reviewed changes

...n/src/commonMain/kotlin/network/bisq/mobile/presentation/ui/uicases/trades/MyTradesScreen.kt Show resolved Hide resolved

rodvar requested changes Nov 25, 2024

View reviewed changes

HenrikJannsen mentioned this pull request Nov 26, 2024

Add offerbook domain #72

Closed

nostrbuddha force-pushed the ui_iter_2 branch from 4f48ba6 to 6ef5893 Compare November 26, 2024 07:21

nostrbuddha added 7 commits November 26, 2024 21:37

My Trades UI 1/3

2fe3b3e

Buy/Sell UI

a569dcc

- Dialog component - Exchange screen improvements - Offerbook UI - Basic

- CurrencyRepository; Dialog component; Improved CurrencyListScreen …

e1a8061

…and OfferListScreen

- minor fixes: coil3 in .toml; Misc

1d607d9

- minor cleanups - removed dead code, updated TODO comments

69ceb42

- added translation keys/strings to CurrencyList/OfferList screens

856a088

- added translation keys/strings to CurrencyList/OfferList screens (m…

14a021c

…issed staging the files)

rodvar force-pushed the ui_iter_2 branch from 6ef5893 to 14a021c Compare November 26, 2024 10:37

rodvar approved these changes Nov 26, 2024

View reviewed changes

- replace attach call with custom composable RememberPresenterLifecy…

c0cab9c

…cle which handle both attach and unattach

rodvar merged commit 3c8ce93 into bisq-network:main Nov 26, 2024
1 check passed

nostrbuddha deleted the ui_iter_2 branch December 4, 2024 11:32

nostrbuddha mentioned this pull request Dec 10, 2024

For Cycle 65 bisq-network/compensation#1684

Closed

		@@ -0,0 +1,26 @@
		package network.bisq.mobile.domain.data.model

		// TODO: Update later


		val myTrades: List<BisqOffer> = presenter.myTrades.collectAsState().value

		LaunchedEffect(Unit) {

UI iteration 2 #73

UI iteration 2 #73

Conversation

nostrbuddha commented Nov 25, 2024

rodvar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nostrbuddha Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nostrbuddha commented Nov 26, 2024

HenrikJannsen commented Nov 26, 2024

nostrbuddha commented Nov 26, 2024 • edited Loading

rodvar commented Nov 26, 2024

rodvar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nostrbuddha Nov 27, 2024 •

edited

Loading

nostrbuddha commented Nov 26, 2024 •

edited

Loading