Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[MX-150] New search page analytics #2043

Merged
merged 3 commits into from
Jan 16, 2020
Merged

[MX-150] New search page analytics #2043

merged 3 commits into from
Jan 16, 2020

Conversation

ds300
Copy link
Contributor

@ds300 ds300 commented Jan 16, 2020

https://artsyproduct.atlassian.net/browse/MX-150

This copies the previous behaviour, except we no longer have a "search screen closed" event because it's a tab now.

Worth noting that there are new subtleties which are not yet being taken into account (e.g. tapping on recent searches vs current search results). We can follow up with @anipetrov on how that should be represented.

Comment on lines -16 to -25
beforeEach(() => {
;(useTracking as jest.Mock).mockImplementation(() => {
return {
trackEvent,
}
})
})
afterEach(() => {
jest.clearAllMocks()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved equivalent code to setupJest.ts

const values = Object.keys(Schema.ActionNames).map(k => Schema.ActionNames[k as any])
values.map(name => expect(name[0] === name[0].toLowerCase()).toBeTruthy())
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test broke but it's only enforcing an old convention that we broke here to maintain compatibility with eigen's analytics schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(so i just deleted it)

Copy link
Contributor

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks David 🙇

@@ -67,6 +68,7 @@ export const Input = React.forwardRef<TextInput, InputProps & TextInputProps>(
input.current.clear()
setValue("")
rest.onChangeText?.("")
rest.onClear?.()
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh neat! The equivalent syntax on Swift would be rest.onClear?(). I'll have to adjust my internal code parser 😛

@artsy-peril artsy-peril bot merged commit 4f50f8c into master Jan 16, 2020
@ashfurrow ashfurrow deleted the new-search-analytics branch January 16, 2020 20:24
@anipetrov
Copy link

left a few comments here https://artsyproduct.atlassian.net/browse/MX-150 -- will be easier to give more specific schema when i can play around with the new experience in the beta -- if i am not mistaken it's not yet in it, is it?

@ashfurrow
Copy link
Contributor

That’s right – should be in the next beta!

@artsyit
Copy link
Collaborator

artsyit commented Jan 17, 2020

🚀 PR was released in v1.21.10 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants