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.

Sign up for GitHub

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

fix: Track transaction item open events #2602

Merged
merged 3 commits into from
Oct 12, 2023
Merged

fix: Track transaction item open events #2602

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc30308
fix: Only track fetch details event if tx details are fetched
usame-algan Oct 9, 2023
8f61a7e
fix: Track accordion open instead of tx details fetch, rename event
usame-algan Oct 9, 2023
50dbcc0
fix: Track every accordion open
usame-algan Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/components/transactions/TxDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ const TxDetails = ({

const [txDetailsData, error, loading] = useAsync<TransactionDetails>(
async () => {
trackEvent(TX_LIST_EVENTS.FETCH_DETAILS)
if (!txDetails) {
Copy link
Member

Choose a reason for hiding this comment

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

That's a negation, isn't it? Why do we even track this event? We should track clicks on the accordeon, not the fetch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also considered tracking this event on the accordion click but with the current structure it doesn't seem possible since the fetch is in a nested component and with the accordion click we can only know if it was opened or closed but not if we already fetched the data before. Do you think we should track the first accordion open in general regardless of the fetch status?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I think we should track the user intention, not how our app interpreted it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it so we track the accordion open once. I also renamed the event to better fit the new purpose.

trackEvent(TX_LIST_EVENTS.FETCH_DETAILS)
}

return txDetails || getTransactionDetails(chainId, txSummary.id)
},
Expand Down
Loading