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

fix(insights): control auto sending events w/ isAutoSendingHitsViewEvents #314

Merged
merged 4 commits into from
May 16, 2024

Conversation

aallam
Copy link
Member

@aallam aallam commented May 14, 2024

Summary

Add isAutoSendingHitsViewEvents to HitsSearcher. If set to true insights view events are automatically sent.
The parameter defaults to false.

FX-2875

@aallam aallam changed the title fix(insights): add a parameter to enable/disable auto sending events fix(insights): add isAutoSendingHitsViewEvents to enable/disable auto sending events May 14, 2024
@aallam aallam changed the title fix(insights): add isAutoSendingHitsViewEvents to enable/disable auto sending events fix(insights): control auto sending events w/ isAutoSendingHitsViewEvents May 14, 2024
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

implementation looks good, just double checking if it could be breaking

Comment on lines +166 to +168
if isAutoSendingHitsViewEvents {
searcher.eventTracker.trackView(for: response.hits, eventName: "Hits Viewed")
}
Copy link
Member

@francoischalifour francoischalifour May 16, 2024

Choose a reason for hiding this comment

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

Could we rather send the view events as batches when the hits enter the viewport?

I'm afraid this PR solution creates more noise, especially for Personalization, which is our only product relying on search view events AFAIK. It blurs the signal because there's no guarantee that the user actually sees all the hits from the response.

I created a more correct approach in JavaScript for the Personalization Storefront, perhaps the principle could be reused?

Copy link
Member Author

Choose a reason for hiding this comment

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

The library doesn't provide a UI components to display hits as far as I know :/
But indeed, this is something we can investigate later on.

@aallam aallam merged commit 8573411 into master May 16, 2024
8 checks passed
@aallam aallam deleted the fix/auto-events branch May 16, 2024 12:39
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.

3 participants