-
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
Wallet: notification for sending collectibles to unpreferred network #20746
Conversation
Jenkins Builds
|
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.
Hi Omar!
Thanks for your PR! I left some comments
:testnet-enabled? (rf/sub [:profile/test-networks-enabled?]) | ||
:goerli-enabled? (rf/sub [:profile/is-goerli-enabled?])}) |
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.
Aren't we getting a warning about subscribing outside of a reactive context with these?
(rf/sub [:wallet/wallet-send-tx-type]))) | ||
send-transaction-data (rf/sub [:wallet/wallet-send]) |
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 not common to subscribe in event callbacks 🤔 I think we should avoid it. Please check https://day8.github.io/re-frame/FAQs/UseASubscriptionInAnEventHandler/#the-root-problem
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.
Thanks @ulisesmac! Refactored the function to an event
:contract-id | ||
:chain-id) | ||
collectible-to-unpreferred? (when collectible-tx? | ||
(not (some #(= collectible-network %) receiver-networks))) |
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.
not-any?
recipient {:label | ||
(utils/get-shortened-address | ||
splitted-address) | ||
:recipient-type :address}] |
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.
The code style here looks very uncommon. consider extracting a call to another binding so that this map is formatted consistently:
Either:
{:a 'a
:b 'b}
or (ugly):
{:a
'a
:b
'b}
But mixing these is not great
9befbf5
to
1963d47
Compare
Unfortunately I think we should hold this pr given that by 2.31 we will look to remove this complex router feature. It is likely we will remove it again in a few weeks. Given that information do we think it is worth adding now? |
Then IMO we shouldn't merge this PR, not merging means less work to do, otherwise, we will need to open an issue to remove the code added in this PR. |
@OmarBasem - can we close this pr for now? 🙏 |
fixes: #20257
Summary
This PR implements showing notification alert when sending a collectible to an unpreferred network by the receiver.
Demo
Screen_Recording_20240715_113011_Status.mp4