-
Notifications
You must be signed in to change notification settings - Fork 573
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(DIA-815): add AuthImpression and ResetYourPassword events (and more!) #10978
Conversation
@@ -74,3 +82,10 @@ export const AuthModal: React.FC = ({ children }) => { | |||
// </KeyboardAvoidingView> | |||
) | |||
} | |||
|
|||
const tracks = { |
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.
#blocking @iskounen - we've been migrating away from this pattern across all of our codebases in favor of something a little easier to work with, test etc.
For any given "app", we should create an associated useAppNameTracking
hook. And then in there we drop all the tracking calls in a form similar to
- https://github.com/artsy/eigen/blob/main/src/app/Scenes/HomeView/useHomeViewTracking.ts.
By grouping in this way, we can consolidate all tracking behavior in one thing, which keeps it tidy and easier to maintain through time. And from there, test wise, we can simplify:
At call sites in component code can do something like
import { useAppNameTracking } from 'path/to/useAppNameTracking'
// SomeComponent.test.tsx
jest.mock('path/to/useAppNameTracking')
...
const mockUseAppNameTracking = useAppNameTracking as jest.Mock
...
it('tracks clicks', () => {
const spy = jest.fn()
mockUseAppNameTracking.mockReturnValue({ trackSomeThing: spy })
thing.fireEvent('click')
expect(spy).toHaveBeenCalled()
})
And then in the useAppNameTracking.tests.ts
file, the tests can test actual properties by simply invoking each of the tracking functions individually and asserting the return values.
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.
I thought tracks
was the latest pattern, so thanks for clarifying that.
There are a few tracking calls in ./src/app/store/AuthModel.ts that I want to switch to this pattern, but I can't use a hook there since it's not a React component. How would you handle those tracking calls?
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 that case (ie, outside of a given subapp), we just leave it alone -- the hook should only be used within the scope of a given "app". (The idea is to make things better, but it doesn't necessarily have to be totally uniform)
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.
Sounds good 👍
}) | ||
} else { | ||
tracking.trackEvent(tracks.resetYourPassword()) |
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.
By rolling tracking up in this way we're constraining all of the different APIs into one function call:
const tracking = useAppNameTracking()
...
onClick={() => tracking.resetYourPassword()}
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.
I added the AuthImpression
event to the tracking hook, but I decided to move this tracking event out of the Form and put it into the AuthModel handler (where the CreatedAccount
and SuccessfullyLoggedIn
events are fired) where it should have been this whole time.
This PR resolves DIA-815
Description
AuthTrigger
schemaAuthImpression
eventtrigger
property toAuthImpression
ResetYourPassword
eventDetermine whether we can surmise a signup or login from the social sign-in serviceHandle TODO in onSocialLoginPR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.