-
Notifications
You must be signed in to change notification settings - Fork 985
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
Wallet: Buy Tokens drawer #17654
Wallet: Buy Tokens drawer #17654
Conversation
Jenkins BuildsClick to see older builds (18)
|
b74e439
to
78bc185
Compare
[icons/icon icon | ||
{:size icon-size | ||
:color icon-color}]])) | ||
(if (= (type icon) Keyword) |
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.
Can use keyword?
(keyword? :x)
;;=> true
(keyword? true)
;;=> false
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.
perhaps we can be even stricter and check for icon keywords, i.e :i/*
🤔
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.
@J-Son89 And what if it is not an icon keyword? I am basically checking if it's a keyword to return an icon
, otherwise return an image
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.
🤷♂️ yeah probably not so much value in it. Better to have that checked by some other mechanism that will be less destructive :)
:color icon-color}]] | ||
[rn/image | ||
{:source icon | ||
:style {:width 32 :height 32}}]))) |
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.
icon has prop :size, can we use this instead?
:height "100%" | ||
:justify-content :flex-start}) | ||
{:margin-horizontal 12 | ||
:flex 1 |
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.
can you give a quick check of other uses of this component to ensure it's not breaking its uses 👍
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.
checked it, all fine 👍
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.
🙌
@@ -91,7 +91,7 @@ | |||
[{:keys [action action-props blur? theme]}] | |||
[rn/view {:style {:margin-left 12}} | |||
(case action | |||
:arrow [icon/icon :i/chevron-right (style/color blur? theme)] | |||
:arrow [icon/icon (or (:icon action-props) :i/chevron-right) (style/color blur? theme)] |
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.
removing this default might break other uses? perhaps you have to set it as a default value here?
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 am not removing anything, I am adding a dynamic icon or a default
:)
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.
aha! nice :)
:moonpay (js/require "../resources/images/services/Moonpay.png") | ||
:ramp (js/require "../resources/images/services/Ramp.png")}) | ||
|
||
(defn get-service-image |
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.
resources/get-service
is probably fine ( imo 🙂)
@@ -201,3 +201,41 @@ | |||
:name "My savings" | |||
:address "0x43c...98d" | |||
:networks [{:name :ethereum :short :eth}]}]) | |||
|
|||
(def buy-tokens-list | |||
[{:title "Ramp" |
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.
curious where do we actually get this info from?
Is it a predefined list or something we should get from status go? 🤔
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.
It's from the designs
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.
Sure, I get that. I am wondering if this is a finalized list. If so we can move it out of temp folder.
Additionally if this data is available from status-go then I can create a follow up issue for someone to get this data from the correct point.
Can you check with design/desktop of where the source of this data is?
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.
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.
awesome, thanks @OmarBasem :D
78bc185
to
98d3710
Compare
86% of end-end tests have passed
Failed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (37)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
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.
Looks good ✨
a440486
to
d639f18
Compare
d639f18
to
5cf8711
Compare
fixes: #17644
This PR implements "Buy tokens" drawer.
Designs
Demo:
RPReplay-Final1697514385.MP4