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

feat(widget): google analytics dimension for injected widget app id #3272

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

shoom3301
Copy link
Collaborator

Summary

  1. Put injected widget appId to appData as an app code
  2. Put injected widget appId to google analytics dimension

@shoom3301 shoom3301 requested a review from a team October 26, 2023 09:03
@shoom3301 shoom3301 self-assigned this Oct 26, 2023
@vercel
Copy link

vercel bot commented Oct 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview Oct 26, 2023 0:35am

🌃 Cosmos ↗︎

@shoom3301 shoom3301 changed the title Widget UI 16 feat(widget): google analytics dimension for injected widget app id Oct 26, 2023
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Soft approve

@@ -93,6 +96,15 @@ export function useAnalyticsReporter() {
googleAnalytics.setDimension(Dimensions.market, marketDimension)
}, [marketDimension])

useEffect(() => {
// Custom dimension 6 - injected widget app id
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add it also in Google Analytics side?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought it was your idea at the last meeting 😄

const APP_CODE = process.env.REACT_APP_APP_CODE

export function useAppData(): AppDataInfo | null {
return useAtomValue(appDataInfoAtom)
}

export function useAppCode(): string | null {
const injectedWidgetMetaData = useAtomValue(injectedWidgetMetaDataAtom)
const injectedWidgetMetaData = useInjectedWidgetMetaData()
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick

not comming from this PR, but in principle not sure we want to make all modules aware there's an injected mode and instead is the client of this module informing one time whats the appKey

This way, in injected mode, it will inject the appData, otherwise it will get it from the env.

Otherwise, we leak a lot of CoW Swap logic here. Imagine we want to promote this module to a lib and use for other projects, then the injected mode would be bothering us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I also feel it's not the best solution. However, I don't see any better ways

@shoom3301 shoom3301 merged commit 022c76d into widget-ui-15 Oct 27, 2023
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2023
@alfetopito alfetopito deleted the widget-ui-16 branch October 31, 2023 10:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants