[Feature] Add analytics passthroughs for Alert #296
Conversation
…arin-alert-analytics
| @@ -43,13 +43,19 @@ export type AlertProps = { | |||
| header: string | |||
| /** True if Alert should start expanded. Defaults to false */ | |||
| initializeExpanded?: boolean | |||
| /** Optional passthrough function for expand event */ | |||
| onExpand?: () => void | |||
There was a problem hiding this comment.
Not sure if we wanted to have an analytics object like with Link here. I think an object is a good idea for: consistency across components, setting it aside as non-functionality changing set of props, and more flexibility expanding analytics for a component in the future if any edge cases warrant it.
If doing that, probably fine to drop the analytics prop in the general list and not worry about locking down apps sending analytics that aren't possible depending on how they functionally configured it--as it's non-functional, I see no issue dropping them on the floor. It might even simplify how apps leverage it to just have a wrapper that always sends expand/collapse analytics even when they can't be hit.
There was a problem hiding this comment.
Hmm. I was going back and forth on this as well. I think it makes sense to add the analytics object and have it limited to events that don't have their own obvious onPress. Link sort of breaks this proposed pattern since it includes analytics.onPress though. We could potentially remove it if it's not already being used.
Added analytics object and moved onExpand and onCollapse inside.
There was a problem hiding this comment.
Link sort of breaks this proposed pattern since it includes analytics.onPress though. We could potentially remove it if it's not already being used.
It is needed in Link due to us handling the onPress logic on Link variants where we can. Outside attactments/custom/calendar where they have to, they are unlikely to send custom onPress logic where they could do onPress analytics since doing so would overwrite the built in onPress behavior.
| /** Optional passthrough function for expand event */ | ||
| onExpand?: () => void | ||
| /** Optional passthrough function for collapse event */ | ||
| onCollapse?: () => void |
There was a problem hiding this comment.
Relatedly to the above comment, don't feel strongly but if we go the analytics object route, I wonder if we should include the Buttons? Link had it so they could send it via analytics or their own onPress logic they have to include--not sure if we should retain that flexibility or leave Link as a one-off due to the cases where we handled the onPress and needed a passthrough.
Description of Change
Add analytics support by adding passthrough functions for expand and collapse. Analytics for buttons should already be covered by the onPresses for each button.
This also includes a fix for the Link background color getting stuck onPress
Testing Packages
N/A
Screenshots/Video
iOS
RPReplay_Final1713893239.MP4
Android
screen-20240423-102900.mp4
Web
Screen.Recording.2024-04-23.at.10.25.43.AM.mov
Testing
PR Checklist
Code reviewer validation:
changeloglabel applied if it's to be included in the changelogPublish
If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:
maininto branchmain