Add recurrent tab in wallet buy assets drawer #20063
Merged
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 (42)
|
ajayesivan
force-pushed
the
19804-buy-assets-recurrent-tab
branch
2 times, most recently
from
b0d2156
to
c6e0426
Compare
|
To avoid unnecessary conflicts I will update and move this PR to review after merging this: #20078. |
dlipicar
added
swap-team
E:M Wallet Crypto Recurring Purchases
and removed
swap-team
E:M Wallet Crypto Recurring Purchases
labels
ajayesivan
force-pushed
the
19804-buy-assets-recurrent-tab
branch
2 times, most recently
from
92fd15e
to
c53695e
Compare
ajayesivan
requested review from
briansztamfater,
smohamedjavid,
J-Son89 and
mmilad75
Comment on lines
+41
to
+46
| [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.
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.
Yes. All the platforms may not support recurrent purchases and we want to preserve the drawer height when changing tabs.
ajayesivan
force-pushed
the
19804-buy-assets-recurrent-tab
branch
2 times, most recently
from
af89f4c
to
2e1fb9c
Compare
ajayesivan
requested review from
smohamedjavid,
ilmotta,
alwx,
flexsurfer,
clauxx and
BalogunofAfrica
ajayesivan
force-pushed
the
19804-buy-assets-recurrent-tab
branch
from
2e1fb9c
to
7adc5cb
Compare
clauxx
reviewed
May 29, 2024
ilmotta
approved these changes
May 29, 2024
| :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.
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.
my vote is for going with dispatch 👍
There was a problem hiding this comment.
@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.
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.
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.
Oh okay, they're tangled, I missed that. Agreed with Jamie's suggestion 👍🏼
There was a problem hiding this comment.
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.
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.
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.
Really appreciate the suggestions. I will update the code. Thank you @ilmotta!
ajayesivan
force-pushed
the
19804-buy-assets-recurrent-tab
branch
2 times, most recently
from
b13774e
to
d72f698
Compare
45% of end-end tests have passedNot 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 passedNot 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 passedNot 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. |
ajayesivan
force-pushed
the
19804-buy-assets-recurrent-tab
branch
from
d72f698
to
00258f2
Compare
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 #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-assetsfeature flag is enabled (Settings -> Feature Flags)Screenshots
status: ready