feat: select asset screen ui #17780
Merged
feat: select asset screen ui #17780
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (92)
|
briansztamfater
force-pushed
the
feat/address-suggestions
branch
from
dcd3498
to
1fae0c9
Compare
briansztamfater
force-pushed
the
feat/select-asset-ui
branch
from
518df6e
to
2fdefba
Compare
briansztamfater
force-pushed
the
feat/address-suggestions
branch
from
63e9661
to
c9458bb
Compare
briansztamfater
force-pushed
the
feat/select-asset-ui
branch
from
8009d9e
to
c26eed8
Compare
briansztamfater
changed the title
briansztamfater
requested review from
vkjr,
tumanov-alex,
smohamedjavid,
J-Son89 and
mmilad75
This was referenced Nov 3, 2023
briansztamfater
force-pushed
the
feat/address-suggestions
branch
2 times, most recently
from
7ce159f
to
42eb3a0
Compare
J-Son89
reviewed
Nov 7, 2023
J-Son89
reviewed
Nov 7, 2023
J-Son89
reviewed
Nov 7, 2023
J-Son89
reviewed
Nov 7, 2023
| @@ -85,7 +85,7 @@ | |||
| :customization-color :blue}] | |||
| [quo/wallet-graph {:time-frame :empty}] | |||
| [quo/wallet-ctas | |||
| {:send-action #(rf/dispatch [:open-modal :wallet-select-address]) | |||
| {:send-action #(rf/dispatch [:open-modal :wallet-select-address (:address account)]) | |||
There was a problem hiding this comment.
should we make the params a map here?
:open-modal :wallet-select-address {:address (:address account)}
There was a problem hiding this comment.
Would be cleaner, agree. But BTW, wondering if we actually need to pass this in navigation params, or maybe would be a better idea to handle this in the app db with subs and events, because either way the user will be able to switch accounts while being inside the send flow
There was a problem hiding this comment.
yeah that's a fair point. We could also do as a follow up if it's too much effort here? 🤔
There was a problem hiding this comment.
I think it is not needed so I removed, we can create a follow up in case we need it later
J-Son89
reviewed
Nov 7, 2023
| @@ -41,70 +44,155 @@ | |||
| :container-style style/empty-container-style}])) | |||
|
|
|||
| (defn- address-input | |||
There was a problem hiding this comment.
can be done outside this pr, I believe we can make this a common component as the same component is used on watch-only select address screen too 👍
J-Son89
reviewed
Nov 7, 2023
J-Son89
reviewed
Nov 7, 2023
| [tab-view @selected-tab]]))) | ||
| (let [valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?]))] | ||
| [rn/scroll-view | ||
| {:content-container-style (style/container margin-top) |
There was a problem hiding this comment.
can margin-top be achieve with insets true in the screens.cljs file?
There was a problem hiding this comment.
Can be replaced with SafeAreaView which should handle this automatically
J-Son89
reviewed
Nov 7, 2023
| {:content-container-style (style/container margin-top) | ||
| :keyboard-should-persist-taps :handled | ||
| :scroll-enabled false} | ||
| [quo/page-nav |
There was a problem hiding this comment.
beyond this pr and beyond 1.27 priorities. seems we should eventually make this a common connected component for wallet with all of the functionality built in for wallet page nav. 👍
cc @smohamedjavid, @OmarBasem @ulisesmac @mmilad75
J-Son89
reviewed
Nov 7, 2023
| (if (or @input-focused? (> (count @input-value) 0)) | ||
| [rn/keyboard-avoiding-view | ||
| {:style {:flex 1} | ||
| :keyboard-vertical-offset 26} |
There was a problem hiding this comment.
where are you getting this 26 from? 🤔 anyway I think when we merge #17737 we can use this here instead 👍
J-Son89
reviewed
Nov 7, 2023
|
Thanks @briansztamfater! |
73% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (33)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
75% of end-end tests have passedFailed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (6)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
|
qoqobolo
force-pushed
the
feat/select-asset-ui
branch
from
8f0e4eb
to
21f3f1c
Compare
|
Hi @briansztamfater, thanks again for waiting! |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/select-asset-ui
branch
from
21f3f1c
to
af4ad5f
Compare
It is fine, we will do a design review later so we can skip this part for now. Thanks for testing! |
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
feat: select asset screen ui Signed-off-by: Brian Sztamfater <brian@status.im>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #16914
Summary
This PR integrates asset selection page UI, using fetched data from status-go and filtering through search input
Platforms
Functional
Steps to test
status: ready