Skip to content
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

Conversion tracking for Banxa and Simplex #5368

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

samholmes
Copy link
Contributor

@samholmes samholmes commented Nov 21, 2024

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

@@ -1463,11 +1463,13 @@ const strings = {
fiat_plugin_fetching_assets: 'Fetching supported assets',
fiat_plugin_sell_cancelled: 'Sell order cancelled',
fiat_plugin_finalizing_quote: 'Finalizing your exchange quote. Please wait as this may take up to a minute',
fiat_plugin_buy_cancelled: 'Buy order cancelled',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Common misspelling. In American English, it's actually "canceled."

Recommend changing all these here and in the respective jsons. See transaction_details_swap_order_cancel

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, change transaction_details_swap_order_cancel to use two L's, since Americans don't really follow American spelling for this, anyway.

Either way, just make sure we're consistent.


switch (link.query.status) {
case 'success': {
console.log('Banxa WebView launch buy success: ' + link.uri)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already log tracking events, don't need to mention "buy success."

Remove completely, imo, our logging is so noisy and there's no real value to this considering we log the event immediately below.

To capture logging for the other cases, consider adding new logging events (not as conversions of course) and sync up with Peter. Adding these events would help with analytics on why/if things fail and if people are backing out of their orders for some reason.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't add new events, but that can be a separate task. Please create the task and tag Peter and I on it as collaborators.

break
}
case 'cancelled': {
console.log('Banxa WebView launch buy cancelled: ' + link.uri)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decided to correct the "canceled" string spelling, change it here too for searchability.


switch (link.query.status) {
case 'success': {
console.log('Simplex WebView launch buy success: ' + link.uri)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See banxa comments

@@ -668,7 +668,7 @@
"transaction_details_fee_warning": "High Network Fees",
"transaction_details_swap": "Swap Funds",
"transaction_details_swap_network_fee": "Swap Network Fee",
"transaction_details_swap_order_cancel": "Swap Order Canceled",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other languages?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to replace this string in other languages that are untranslated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "replace the string"?


switch (link.query.status) {
case 'success': {
console.log('Banxa WebView launch buy success: ' + link.uri)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't add new events, but that can be a separate task. Please create the task and tag Peter and I on it as collaborators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants