-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add withTracking and useTracking, post beacon launch fixes #1294
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
base: develop
Are you sure you want to change the base?
Conversation
packages/snap-preact-components/src/components/Molecules/Result/Result.test.tsx
Outdated
Show resolved
Hide resolved
packages/snap-preact-components/src/components/Trackers/ResultTracker/ResultTracker.tsx
Outdated
Show resolved
Hide resolved
Test is failing... perhaps now is a good time to re-attempt making this test stable. |
The past 4 actions have been successful. 🤞 removeEventListener requires a reference to a function but then you can't pass in a controller and I don't see a way to refactor this. I think it would be fine without removing or keeping it as is. A new instance of withTracking is created for each Result element so should be fine? Or maybe upon rerender, the instance of withTracking isn't recreated and having the removeEventListener call would then make sense? I also refactored some of the same useIntersectionAdvanced code into a single createImpressionObserver function. At least the observer parameters are defined in a single spot now. The usage across the 3 varies so that why this also doesn't include the tracking call itself. |
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.
In a recent discussion we talked about the possibility of removing options for doing the tracking integration (eg. getting rid of the hook or other usages). However, we need to keep <ResultTracker>
for legacy purposes (for profile and result recommendations tracking as many styles exist associated with these wrappers).
That leaves the HOC and the hook - and I think here we should keep both.
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.
Tests seem reliable.
I played around with this a bit - how is it handling the error responses from the API now? I notice it doesn't throw errors when I block the request anymore... 🤔
packages/snap-preact-demo/tests/cypress/e2e/tracking/track.cy.js
Outdated
Show resolved
Hide resolved
packages/snap-store-mobx/src/Autocomplete/Stores/AutocompleteQueryStore.ts
Show resolved
Hide resolved
….html, update snapi-types
console.warn( | ||
'tracker.cart.view is deprecated and no longer functional. Use tracker.events.cart.add() and tracker.events.cart.remove() instead' | ||
); | ||
this.events.error.snap({ data: { message: `tracker.track.cart.view was called`, details: { fromAttribute: Boolean(fromAttribute) } } }); |
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.
Nice addition with the error logger - I'm not sure we care that these events are not being sent anymore, but this will be a good test of getting errors in this way.
No description provided.