Introduce Wallet main pages #389
Conversation
johnny9
force-pushed
the
wallets
branch
2 times, most recently
from
19c8cd8
to
fd5704d
Compare
There was a problem hiding this comment.
tACK fd5704d
So far so good, I like it.
A few observations:
1. First screen after onboarding or once onboarded when app starts (wallet loaded/ no wallet).
I see there's no difference if we have a wallet loaded or not (I haven't checked the code yet), I guess this is why the PR is still on draft, in Figma I found this model (please check top left small wallet icon):
Should we don't have a splash-screen or initial window as we have in QT project?
Checking in Figma, I couldn't find any reference for such thing, perhaps I missed or was already discused that was not necessary.
2. BlockClock small icon at the top taskbar (?) and BlockClock details view as in Figma.
The BlockCount looks like fixed at the top and should be centered as it's shown on Figma:
Also, the top BlockCount small icon looks a bit empty as is, I see in Figma there's a (÷) symbol inside it (?):
That's when the app is still trying to sync, not sure when it's completed (100% as the 1st screenshot I put on this bullet point), how the BlockCount small icon would look like, I couldn't find one in Figma among the options:
Figma design source for reference.
Code-wise, is it possible to split it in different and more modular commits?
nit: "qml:" prefix in the PRs title isn't necessary.
cc @GBKS for his opinion on the obs.
|
Just tested because of the mention and looks like a great start. Lots of details to tweak, of course, as pablo pointed out, but it's also a draft PR. Is there anything specific to give feedback on at the moment? |
There was a problem hiding this comment.
This is definitely moving us in a correct direction towards implementing the 1.2 milestone. In the feature set that this adds, the requirement for merging should be to make sure that the elements adhere to the design file spec, so we don't need to have any fix-ups on that afterwards.
Before moving to deep review, this should be split up into logical commits. This is just too many changes for one commit.
- Each icon added can be it's own commit, with proper title and description
- The functionality of knowing if the wallet is enabled should be its own commit; optionally this could be split off from this PR, into a different PR that adds this functionality along with introducing the wallet navbar component
- Adding the ability to optionally show the NetworkIndicator in the Block Clock should be its own commit
- the mini block clock dial should be its own commit; optionally can be split off into it's own PR
- Important controls added can be contained in their own commits
- I don't love the naming/structure of MobileWallets/DesktopWallets.qml :)
I would recommend you remove the wallet badge and detection of ENABLE_WALLET and split that off into a separate PR that is based on this (then) leaner PR. In this PR instead of addressing the badge and ENABLE_WALLET and what that entails for the views, we will temporarily (until the next PR) just always show the new activity page. Here, we put more emphasis on getting the small block clock correct, according to the design. In the next PR we add a proper badge and detection of ENABLE_WALLET (plus all that entails for the view).
The above has certain implications that I think will help us get things right, so here we focus on:
- The new desktop layout
- Correct implementation of the mini block clock
- We don't address the mobile layout, because that's incredibly wallet badge heavy, and we want to get that right in a separate pr
- We don't include any wallet badge, or detection of ENABLE_WALLET
By slimming this down, we can discuss in separate PR's the:
- implementation of a proper wallet badge
- Discuss how we want to separate desktop and wallet views, this is a big move as we've tried to keep everything cross-compatible, so a separate PR on this keeps conversations in the right place
There was a problem hiding this comment.
tACK fd5704d on Ubuntu 22.04 and Android (armv7)
Great progress! Looks like the base is there for wallet functionality
On Ubuntu:
img(
)
Gotta say the Android version looks great:
*white bar appeared after capturing the screen...
This icon is used in the navigation bar in Wallet mode
johnny9
force-pushed
the
wallets
branch
2 times, most recently
from
cf025f7
to
b5a1356
Compare
There was a problem hiding this comment.
did some manual testing on desktop, looks good
-
there is a bug where sometimes the gear-outline icon is highlighted, while not selected. this can happen after having minimized the window. (happened twice, not sure exactly how to repro)
-
nit: the animation when moving hover away from settings (small flash) & removing selection from settings is different than the others
Screencast.from.02-04-2024.10.57.34.webm
This is a main page when the AppMode is Desktop with walletEnabled. The page contains a tab based layout with the sub pages being selectable by clicking on a NavigationTab in the NavigationBar at the top of the page.
The network indicator is redundant on the block clock page when in Desktop wallet mode.
|
Update from b5a1356 to 37e3af6
|
I haven't had this happen yet but i wonder if it was fixed when i fixed the color change animation for it where it was going blank in transition |
There was a problem hiding this comment.
re tACK 37e3af6
Changes since my last review:
- ✅ Code change was split in separate commits (checked them all), which looks much better organised now.
- ✅ `BlockClock` looks centered now.
- ✅ I couldn't reproduce the issue reported by @MarnixCroes with the video, perhaps was fixed with the gear icon transitioning to blank (?).
- ✅ looking forward to see this feature: "Removed mini blockclock implementation. Will add it later as a separate PR."
- ⭕ commit 6e42abc (
qml: Add property to hide NetworkIndicator on BlockClock page), what this should do? I think I'm missing something because on Desktop mode, I made it visible (showNetworkIndicator: true) on mainnet but I don't see the difference. Could you please shed some light on this?
This will show the network indicator if its not on mainnet (testnet/signet) on the block clock control. The reason i added the bool is because it is redundant in desktop wallet mode because the indicator is already at the top in the navigation bar. |
Adds DesktopWallets.qml and MobileWallets.qml. These pages are the central navigation points for the application when wallets are enabled.
A core piece of the page is controlling the navigation flow between the sub pages of the wallet application. For the desktop implementation, a new NavigationBar design is used along with a Stack layout to hold the pages. For mobile, a DirectionalStackView is used to push and pop the wallet sub pages based on the UX designed flow.