-
Notifications
You must be signed in to change notification settings - Fork 179
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(protocol-designer): analytics opt in modal fixes #17106
base: chore_release-pd-8.2.1
Are you sure you want to change the base?
Changes from all commits
e99b425
7e7c023
36c07fc
dd0abea
2ed2d81
8131d34
bf5ac76
28298ae
2d77538
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not directly related to this pr but seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll leave that to a follow up unless you think its affecting the modal? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -499,7 +499,7 @@ export const trackEventMiddleware: Middleware<BaseState, any> = ({ | |
// NOTE: this is the Redux state AFTER the action has been fully dispatched | ||
const state = getState() | ||
|
||
const optedIn = getHasOptedIn(state as BaseState) ?? false | ||
const optedIn = getHasOptedIn(state as BaseState)?.hasOptedIn ?? false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not 100% sure but the request is mixpanel is |
||
const event = reduxActionToAnalyticsEvent(state as BaseState, action) | ||
|
||
if (event != null) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import type { BaseState } from '../types' | ||
export const getHasOptedIn = (state: BaseState): boolean | null => | ||
import type { OptInState } from './reducers' | ||
export const getHasOptedIn = (state: BaseState): OptInState => | ||
state.analytics.hasOptedIn |
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.
is it cool with yall if i remove this? I don't think it'll get too annoying throughout pd development and it didn't seem to be working correctly in production (i tested my PD in prod and even with optIn as undefined, the modal still didn't show up) What do you think @shlokamin @koji @ncdiehl11 ?