Skip to content
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

Feat/building simple send #16868

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Khushboo-dev-cpp
Copy link
Contributor

@Khushboo-dev-cpp Khushboo-dev-cpp commented Dec 4, 2024

fixes #16836

What does the PR do

This PR add some dummy components + some actual ones in order to fix a mechanism for the dialog sizing.

Please not even though the AccountSelector, TokenSelector and NetworkFilter are added in this PR they are not worked on completely yet and is still something that is WIP under Epic #16696

Please note this PR doesn't cover QML Units tests, I will work on them next.
The Recipient selector is only temporary one and not done yet TBD under #16916

Affected areas

SimpleSend.qml

Architecture compliance

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
Screen.Recording.2024-12-04.at.7.14.49.PM.mov
Screen.Recording.2024-12-04.at.7.19.35.PM.mov
Screen.Recording.2024-12-08.at.11.21.18.PM.mov

@status-im-auto
Copy link
Member

status-im-auto commented Dec 4, 2024

Jenkins Builds

Click to see older builds (57)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ d350cfb #2 2024-12-04 17:12:39 ~9 min tests/nim 📄log
✔️ d350cfb #2 2024-12-04 17:13:08 ~9 min macos/aarch64 🍎dmg
d350cfb #2 2024-12-04 17:17:00 ~13 min tests/ui 📄log
✔️ d350cfb #2 2024-12-04 17:19:37 ~16 min linux-nix/x86_64 📦tgz
✔️ d350cfb #2 2024-12-04 17:21:00 ~17 min macos/x86_64 🍎dmg
✔️ d350cfb #2 2024-12-04 17:21:45 ~18 min linux/x86_64 📦tgz
✔️ 7ccfcc1 #4 2024-12-04 17:33:30 ~8 min macos/aarch64 🍎dmg
✔️ 7ccfcc1 #4 2024-12-04 17:35:04 ~9 min tests/nim 📄log
✔️ 7ccfcc1 #4 2024-12-04 17:37:49 ~12 min macos/x86_64 🍎dmg
✔️ 7ccfcc1 #4 2024-12-04 17:42:55 ~17 min tests/ui 📄log
✔️ 7ccfcc1 #4 2024-12-04 17:46:17 ~20 min linux/x86_64 📦tgz
✔️ 7ccfcc1 #4 2024-12-04 17:46:56 ~21 min linux-nix/x86_64 📦tgz
✔️ 7ccfcc1 #4 2024-12-04 17:59:22 ~33 min windows/x86_64 💿exe
✔️ 59b4155 #5 2024-12-04 18:21:44 ~7 min macos/aarch64 🍎dmg
✔️ 9f2a213 #6 2024-12-04 18:27:34 ~5 min macos/aarch64 🍎dmg
✔️ 9f2a213 #6 2024-12-04 18:30:56 ~8 min tests/nim 📄log
✔️ 51fc469 #7 2024-12-04 18:39:28 ~6 min macos/aarch64 🍎dmg
✔️ 51fc469 #7 2024-12-04 18:44:56 ~11 min tests/nim 📄log
✔️ 51fc469 #7 2024-12-04 18:46:40 ~13 min macos/x86_64 🍎dmg
✔️ 51fc469 #7 2024-12-04 18:47:21 ~14 min linux-nix/x86_64 📦tgz
✔️ 51fc469 #7 2024-12-04 18:48:39 ~15 min tests/ui 📄log
✔️ 51fc469 #7 2024-12-04 18:54:51 ~21 min linux/x86_64 📦tgz
✔️ 51fc469 #7 2024-12-04 19:01:10 ~27 min windows/x86_64 💿exe
✔️ 2a789a7 #9 2024-12-04 20:45:24 ~6 min macos/aarch64 🍎dmg
✔️ 2a789a7 #9 2024-12-04 20:47:11 ~7 min tests/nim 📄log
✔️ 2a789a7 #9 2024-12-04 20:52:01 ~12 min tests/ui 📄log
✔️ 2a789a7 #9 2024-12-04 20:53:08 ~13 min linux-nix/x86_64 📦tgz
✔️ 2a789a7 #9 2024-12-04 20:54:06 ~14 min macos/x86_64 🍎dmg
✔️ 2a789a7 #9 2024-12-04 20:55:49 ~16 min linux/x86_64 📦tgz
✔️ 2a789a7 #9 2024-12-04 21:04:30 ~25 min windows/x86_64 💿exe
✔️ 93224cb #10 2024-12-05 09:58:50 ~6 min macos/aarch64 🍎dmg
✔️ 93224cb #10 2024-12-05 10:00:47 ~8 min tests/nim 📄log
✔️ 93224cb #10 2024-12-05 10:04:35 ~11 min tests/ui 📄log
✔️ 93224cb #10 2024-12-05 10:06:29 ~13 min macos/x86_64 🍎dmg
✔️ 93224cb #10 2024-12-05 10:09:40 ~17 min linux-nix/x86_64 📦tgz
✔️ 93224cb #10 2024-12-05 10:15:02 ~22 min linux/x86_64 📦tgz
✔️ 93224cb #10 2024-12-05 10:20:23 ~27 min windows/x86_64 💿exe
✔️ 153f94b #11 2024-12-05 21:47:03 ~7 min macos/aarch64 🍎dmg
✔️ 153f94b #11 2024-12-05 21:48:22 ~9 min tests/nim 📄log
153f94b #11 2024-12-05 21:48:56 ~9 min tests/ui 📄log
✔️ 153f94b #11 2024-12-05 21:52:52 ~13 min macos/x86_64 🍎dmg
✔️ 153f94b #11 2024-12-05 21:53:09 ~14 min linux-nix/x86_64 📦tgz
✔️ 153f94b #11 2024-12-05 21:59:02 ~19 min linux/x86_64 📦tgz
✔️ 153f94b #11 2024-12-05 22:11:04 ~31 min windows/x86_64 💿exe
✔️ 153f94b #12 2024-12-06 08:49:59 ~12 min tests/ui 📄log
✔️ 56dffde #12 2024-12-08 22:35:23 ~5 min macos/aarch64 🍎dmg
✔️ 56dffde #12 2024-12-08 22:36:43 ~7 min tests/nim 📄log
56dffde #13 2024-12-08 22:40:39 ~11 min tests/ui 📄log
✔️ 56dffde #12 2024-12-08 22:43:15 ~13 min linux-nix/x86_64 📦tgz
✔️ 56dffde #12 2024-12-08 22:43:35 ~14 min macos/x86_64 🍎dmg
✔️ 56dffde #12 2024-12-08 22:51:49 ~22 min linux/x86_64 📦tgz
✔️ 56dffde #12 2024-12-08 22:56:59 ~27 min windows/x86_64 💿exe
✔️ 017cf6c #13 2024-12-09 10:23:45 ~7 min tests/nim 📄log
✔️ 017cf6c #13 2024-12-09 10:24:22 ~7 min macos/aarch64 🍎dmg
✔️ 98aeb7e #14 2024-12-09 10:33:26 ~7 min macos/aarch64 🍎dmg
✔️ 98aeb7e #14 2024-12-09 10:33:45 ~7 min tests/nim 📄log
✔️ 98aeb7e #15 2024-12-09 10:38:12 ~11 min tests/ui 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 16e67a2 #15 2024-12-09 10:47:13 ~6 min macos/aarch64 🍎dmg
✔️ 16e67a2 #15 2024-12-09 10:48:07 ~7 min tests/nim 📄log
✔️ 16e67a2 #16 2024-12-09 10:52:11 ~11 min tests/ui 📄log
✔️ 16e67a2 #15 2024-12-09 10:54:53 ~13 min linux-nix/x86_64 📦tgz
✔️ 16e67a2 #15 2024-12-09 10:55:29 ~14 min macos/x86_64 🍎dmg
✔️ 16e67a2 #15 2024-12-09 11:01:57 ~21 min linux/x86_64 📦tgz
✔️ 16e67a2 #15 2024-12-09 11:05:36 ~24 min windows/x86_64 💿exe
✔️ 0272273 #16 2024-12-10 08:30:13 ~6 min macos/aarch64 🍎dmg
✔️ 0272273 #16 2024-12-10 08:31:39 ~7 min tests/nim 📄log
✔️ 0272273 #17 2024-12-10 08:35:37 ~11 min tests/ui 📄log
✔️ 0272273 #16 2024-12-10 08:38:04 ~14 min macos/x86_64 🍎dmg
✔️ 0272273 #16 2024-12-10 08:40:54 ~17 min linux-nix/x86_64 📦tgz
✔️ 0272273 #16 2024-12-10 08:46:20 ~22 min linux/x86_64 📦tgz
✔️ 0272273 #16 2024-12-10 08:50:00 ~26 min windows/x86_64 💿exe

@Khushboo-dev-cpp Khushboo-dev-cpp force-pushed the feat/buildingSimpleSend branch 3 times, most recently from 59b4155 to 9f2a213 Compare December 4, 2024 18:21
@Khushboo-dev-cpp Khushboo-dev-cpp force-pushed the feat/UpdateLaunchSendModalMechanism branch from c52e770 to 89daa99 Compare December 4, 2024 18:26
Base automatically changed from feat/UpdateLaunchSendModalMechanism to master December 4, 2024 20:34
@Khushboo-dev-cpp Khushboo-dev-cpp force-pushed the feat/buildingSimpleSend branch 2 times, most recently from 2a789a7 to 93224cb Compare December 5, 2024 09:52
@Khushboo-dev-cpp Khushboo-dev-cpp marked this pull request as ready for review December 5, 2024 16:11
@Khushboo-dev-cpp Khushboo-dev-cpp requested review from Cuteivist and removed request for a team December 5, 2024 16:11
@benjthayer
Copy link

benjthayer commented Dec 9, 2024

Hey @Khushboo-dev-cpp looks great! Just a couple of things I spotted:

  1. When the sticky header transitions in, is it possible for the asset icon/name and network selector to animate down so it feels like they are connected to the backing with shadow that transitions in? (as opposed to them being masked/revealed in their final Y position):
SendHeader.mp4
Screen.Recording.2024-12-10.at.9.24.52.AM.mov

I think it now works the way you mentioned @benjthayer ;)


  1. If possible, it would be good to avoid this interim state where the sticky header content is visible at the same time as the content it replaces. Perhaps just need to bring the sticky header in a little deeper in the scroll?
Screenshot 2024-12-09 at 10 22 23

Fixed, I bring in the sticky header after a bit of padding.

  1. I never checked if generally this was possible, but is it possible to add the opacity and blur to the footer and header backing so, as the user scrolls, some of the dialog content can be seen through them?

Example in the design:

Screenshot 2024-12-09 at 10 26 22 https://www.figma.com/design/FkFClTCYKf83RJWoifWgoX/Wallet-v2?node-id=25639-170189&t=1EBVs8JT8hv5TdnN-4

I cant see it in design, but I tried some playing with FastBlur and it looks like this

Screen.Recording.2024-12-09.at.10.45.24.PM.mov
  1. The skeleton loading bars are a little thin, their height is designed to match the cap-height of the text they replace
Screenshot 2024-12-09 at 10 10 37

Fixed
image

Example from Figma:
https://www.figma.com/design/FkFClTCYKf83RJWoifWgoX/Wallet-v2?node-id=25657-208573&t=1EBVs8JT8hv5TdnN-4
Screenshot 2024-12-09 at 10 11 54


  1. Something we added via Swap is that the Max button has the ticker of the asset included. This is just to make things more clear what denomination the user is specifying (crypto vs FIAT).

Done.
Screenshot 2024-12-10 at 9 30 59 AM

Screenshot 2024-12-09 at 10 14 28
  1. The amount entry field looks slightly indented compared to the rest of the dialog content. Also the fiat value underneath has a little more padding above it than the designs. In the design, the amount input, stroke and fiat value are grouped with more padding above and below this group than b
Screenshot 2024-12-09 at 10 27 27 etween the elements

https://www.figma.com/design/FkFClTCYKf83RJWoifWgoX/Wallet-v2?node-id=25192-32990&t=1EBVs8JT8hv5TdnN-4
Screenshot 2024-12-09 at 10 15 16

Improved now
Untitled


  1. The network menu overlaps the dropdown button slightly. There should be 4px separation between the button and menu flyout. The hover state for the button should also be Black 5% - this is a slight deviation from the design system to ensure the hover state shows up on the grey backing.
Screenshot 2024-12-09 at 10 17 01

Fixed
image


  1. When the user changes account to send from and there is an asset selected, they should see their balance of that asset on the currently selected network along with each account in the menu:

https://www.figma.com/design/FkFClTCYKf83RJWoifWgoX/Wallet-v2?node-id=25193-64791&t=1EBVs8JT8hv5TdnN-4
Screenshot 2024-12-09 at 10 20 46

Fixed in this task #16834


Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 👍


loading: loadingCheckbox.checked

onReviewSendClicked: console.log("review send clicked")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: The Logs and LogsAndControlsPanel components are not really used. Did you mean to log this in the LogsAndControlsPanel? Otherwise just a simple panel would do. Same on all pages

@@ -32,7 +33,8 @@ Dialog {
anchors.centerIn: Overlay.overlay

padding: 16
margins: 64
// by design
margins: root.contentItem.Window.window.height <= 780 ? 28: 64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need an extensive regression on all modals? Should be harmless, but you never know :D


import AppLayouts.Wallet.views 1.0

Rectangle {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to inherit a Control instead. Gives more freedom for styling or adapting this to different views.

/** Output signal to request signing of the transaction **/
signal reviewSendClicked()
/** Output signal to request fetching fees **/
signal fetchFees()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the SendModal as simple as possible and UI only. Meaning that it doesn't have additional logic like detecting when new fees are required.
If we detect this here, the fees responsibility will be split between two components:

  • Send modal to detect when a new fee is required based on what the user changes in the UI
  • Other component to aggregate the user input from the SendModal and request the fees on fetchFees signal.

Another way would be for the SendModal to become a simple form. Any change in the form can be handled by the user of the SendModal. As a result, the logic of updating the fees is transparently handled in a single place, outside of the SendModal.

WDYT?

**/
required property var flatNetworksModel
/** true if testnet mode is on **/
required property var areTestNetworksEnabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
required property var areTestNetworksEnabled
required property bool areTestNetworksEnabled

required property var areTestNetworksEnabled
/** whether community tokens are shown in send modal
based on a global setting **/
required property var showCommunityAssetsInSend
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
required property var showCommunityAssetsInSend
required property bool showCommunityAssetsInSend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog sizing
4 participants