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

Track token lists #6303

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

Track token lists #6303

wants to merge 10 commits into from

Conversation

greg-schrammel
Copy link
Contributor

@greg-schrammel greg-schrammel commented Dec 5, 2024

Fixes APP-2067

What changed (plus any additional context for devs)

tracks token lists, amount of tokens in the list and how many have icons and price

screen: 'wallet' | 'swap' | 'send';
total_tokens: number;
no_icon: number;
no_price?: number;
query?: string; // query is only sent when searching token to buy/sell in the swap screen

Screen recordings / screenshots

What to test

Copy link

linear bot commented Dec 5, 2024

@brunobar79
Copy link
Member

Launch in simulator or device for af4c29f

Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

LGTM but couple questions about performance

Comment on lines +463 to +475

useEffect(() => {
if (searchCurrencyLists.isLoading) return;
const params = { screen: 'swap' as const, total_tokens: 0, no_icon: 0, query };
for (const assetOrHeader of searchCurrencyLists.results) {
if (assetOrHeader.listItemType === 'header') continue;
if (!assetOrHeader.icon_url) params.no_icon += 1;
params.total_tokens += 1;
}
analyticsV2.track(analyticsV2.event.tokenList, params);
}, [searchCurrencyLists.results, searchCurrencyLists.isLoading, query]);

return searchCurrencyLists;
Copy link
Contributor

Choose a reason for hiding this comment

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

does having a useEffect in here have any negative side effects to re-rendering the list?

If it does, we could mount a separate component that listens to react-query subscriptions and doesn't touch the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it doesn't trigger a rerender ever, just sends track events

@derHowie derHowie marked this pull request as draft December 9, 2024 16:26
@derHowie derHowie removed the request for review from DanielSinclair December 9, 2024 16:26
@greg-schrammel greg-schrammel marked this pull request as ready for review December 9, 2024 18:02
@walmat walmat self-requested a review December 9, 2024 18:02
@greg-schrammel greg-schrammel requested a review from maxbbb December 9, 2024 18:32
@brunobar79
Copy link
Member

Launch in simulator or device for df43c82

Copy link
Member

@derHowie derHowie left a comment

Choose a reason for hiding this comment

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

Hey @greg-schrammel, this doesn't appear to be functioning correctly and may not be covering the ticket's requirements. I saw three issues:

  • the function meant to track assets on the wallet screen doesn't fire
  • the send screen is sending information for the wallet screen
  • I don't believe any tracking was added for the discover screen

src/screens/SendSheet.tsx Outdated Show resolved Hide resolved
src/hooks/useWalletSectionsData.ts Show resolved Hide resolved
@greg-schrammel
Copy link
Contributor Author

Hey @greg-schrammel, this doesn't appear to be functioning correctly and may not be covering the ticket's requirements. I saw three issues:

  • the function meant to track assets on the wallet screen doesn't fire
  • the send screen is sending information for the wallet screen
  • I don't believe any tracking was added for the discover screen

ah the discover screen I was thinking trending tokens but there is a search there, should I add this tracking for that?

@derHowie
Copy link
Member

Hey @greg-schrammel, this doesn't appear to be functioning correctly and may not be covering the ticket's requirements. I saw three issues:

  • the function meant to track assets on the wallet screen doesn't fire
  • the send screen is sending information for the wallet screen
  • I don't believe any tracking was added for the discover screen

ah the discover screen I was thinking trending tokens but there is a search there, should I add this tracking for that?

yes, please

@brunobar79
Copy link
Member

Launch in simulator or device for d9d427e

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.

4 participants