-
Notifications
You must be signed in to change notification settings - Fork 984
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
Add recurrent tab in wallet buy assets drawer #20063
Conversation
Jenkins BuildsClick to see older builds (42)
|
b0d2156
to
c6e0426
Compare
To avoid unnecessary conflicts I will update and move this PR to review after merging this: #20078. |
92fd15e
to
c53695e
Compare
[min-height set-min-height] (rn/use-state 0) | ||
on-layout (rn/use-callback | ||
#(set-min-height | ||
(oops/oget % :nativeEvent :layout :height)))] |
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.
Just wondering why we need to use the on-layout and min-height. Is this intended to maintain the same height for the recurrent
tab (empty for now) as the one-time
tab?
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.
Yes. All the platforms may not support recurrent purchases and we want to preserve the drawer height when changing tabs.
af89f4c
to
2e1fb9c
Compare
2e1fb9c
to
7adc5cb
Compare
:on-press #(rn/open-url site-url)}]) | ||
[{:keys [name description fees logo-url site-url recurrent-site-url]} _ _ {:keys [tab]}] | ||
(let [open-url (rn/use-callback (fn [] | ||
(rn/open-url (if (= tab :recurrent) recurrent-site-url site-url))) |
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.
FYI @seanstrom, @ulisesmac this is an example of what we were discussing yesterday. If we should mostly/only use dispatch
calls in the view and not directly call functions with side-effects. Similar to how we don't call navigation functions directly.
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.
my vote is for going with dispatch 👍
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.
@ilmotta Do you have any notes on this discussion?
I think opening a URL can be a generic event like :navigate-to
that we can use whenever we want to open a URL in an external browser. I'm tempted to add this to navigation/events
. wdyt?
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 is :browser.ui/open-url, although perhaps this can be moved improved upon somehow as this is defied in legacy codebase, probably we should move it? 🤔
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.
As @J-Son89 said @ajayesivan, I think there's a strong preference to create events for the UI layer, even though it can be convenient to directly call certain functions. So yes, just like navigation, we would have an event to open any URL. @mmilad75 recently merged a PR that added an effect to open URL, the only missing piece is an event.
But since Jamie said :browser.ui/open-url
already exists, it's something to consider now.
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.
Oh okay, they're tangled, I missed that. Agreed with Jamie's suggestion 👍🏼
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.
We are using :browser.ui/open-url
in multiple places. If you guys don't mind, I will refactor this in a separate PR.
src/status_im/subs/wallet/buy.cljs
Outdated
:wallet/recurrent-crypto-on-ramps | ||
:<- [:wallet/crypto-on-ramps] | ||
(fn [crypto-on-ramps _] | ||
(filter #(seq (:recurrent-site-url %)) crypto-on-ramps))) |
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 significantly faster to use (not clojure.string/blank? s))
instead of seq
on strings. I say this with relative confidence because I performed micro benchmarks in cljs and in clj and results were quite clear with both small and large strings.
You can then also invert the logic to avoid having to use not
and the code would be:
(rf/reg-sub :wallet/recurrent-crypto-on-ramps
:<- [:wallet/crypto-on-ramps]
(fn [crypto-on-ramps _]
(remove #(string/blank? (:recurrent-site-url %)) crypto-on-ramps)))
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.
Still related, and I don't how much the feedback is useful, but we could store separate pieces of crypto on ramp results (recurrent or on time for example) in the app-db and do the filtering/removal once, at write time (event layer), instead of read time (sub layer). It's just an idea, not a suggestion for change.
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.
Really appreciate the suggestions. I will update the code. Thank you @ilmotta!
b13774e
to
d72f698
Compare
45% of end-end tests have passed
Not executed tests (1)Failed tests (25)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Passed tests (23)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
|
41% of end-end tests have passed
Not executed tests (1)Failed tests (27)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Passed tests (21)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
@ajayesivan thanks for the PR. Let me re-run e2e, as there are lot of failures related to Waku. |
43% of end-end tests have passed
Not executed tests (1)Failed tests (26)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Expected to fail tests (3)Click to expandClass TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (22)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
@ajayesivan currently e2e are failing because of Waku issues, so we cannot get valid results. Given that feature is under the flag, hopefully it should not cause regression, so let's merge the PR. |
d72f698
to
00258f2
Compare
fixes #19804
fixes #20072 (Right now we don't have any recurrent options, so the tab will be empty)
Summary
Adds Recurrent Tab in the buy assets sheet.
Testing notes
Changes are under a feature flag, manual qa & design review can be skipped.
Platforms
Steps to test
Note: Make sure
buy-recurrent-assets
feature flag is enabled (Settings -> Feature Flags)Screenshots
status: ready