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: add a survey to opt out SR #27949

Merged
merged 10 commits into from
Jan 28, 2025
Merged

feat: add a survey to opt out SR #27949

merged 10 commits into from
Jan 28, 2025

Conversation

veryayskiy
Copy link
Contributor

We want to show a survey when customers turn off Session Replay to collect the data why people churn.

Screenshot 2025-01-28 at 11 47 06

surveyId: string
}

export const InternalMultipleChoiceSurveyLogic = kea<InternalMultipleChoiceSurveyLogicType>([
Copy link
Member

Choose a reason for hiding this comment

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

nit, we tend to lower camel case these... so internalMultipleChoiceSurveyLogic

setThankYouMessage: (thankYouMessage: string) => ({ thankYouMessage }),
}),
reducers({
surveyId: [
Copy link
Member

Choose a reason for hiding this comment

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

what's the use case for varying the survey id past the one specified in props?
since we're keyed on survey id in props i think this shouldn't change?

Copy link
Member

Choose a reason for hiding this comment

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

I think I see why you're using this and included what to do instead - hope that makes sense :)

})),
afterMount(({ actions, props }) => {
/** When the logic is mounted, set the surveyId from the props */
actions.setSurveyId(props.surveyId)
Copy link
Member

Choose a reason for hiding this comment

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

ah... you can read props in variious places so don't need that...

let me re-read this...

@@ -316,6 +316,7 @@ export const SESSION_REPLAY_MINIMUM_DURATION_OPTIONS: LemonSelectOptions<number
]

export const UNSUBSCRIBE_SURVEY_ID = '018b6e13-590c-0000-decb-c727a2b3f462'
export const SESSION_RECORDING_OPT_OUT_SURVEY_ID = '0194a763-9a13-0000-8088-32b52acf7156'
Copy link
Member

Choose a reason for hiding this comment

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

we could do this as a flag payload so we can change the survey without a code release but that's very nit-picky

listeners(({ actions, values }) => ({
/** When surveyId is set, get the list of surveys for the user */
setSurveyId: () => {
posthog.getSurveys(actions.handleSurveys)
Copy link
Member

Choose a reason for hiding this comment

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

so you can call this in the afterMount instead of calling setSurveyId in it

},
/** Callback for the surveys response. Filter it to the surveyId and set the survey */
handleSurveys: ({ surveys }) => {
const survey = surveys.find((s: PostHogSurvey) => s.id === values.surveyId)
Copy link
Member

Choose a reason for hiding this comment

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

then this would be props.surveyId

},
],
}),
listeners(({ actions, values }) => ({
Copy link
Member

Choose a reason for hiding this comment

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

you'd add props here alongside actions, values

Comment on lines 67 to 68
/** When surveyId is set, get the list of surveys for the user */
setSurveyId: () => {},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** When surveyId is set, get the list of surveys for the user */
setSurveyId: () => {},

i think we can remove entirely?

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

works locally 👍

i think a visual countdown and close button like on lemon toast would be nice, but that can follow if you agree

Copy link
Contributor

github-actions bot commented Jan 28, 2025

Size Change: -113 B (-0.01%)

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.16 MB -113 B (-0.01%)

compressed-size-action

@veryayskiy veryayskiy merged commit 33ba034 into master Jan 28, 2025
99 checks passed
@veryayskiy veryayskiy deleted the feat/sr/opt-out-survey branch January 28, 2025 17:49
adamleithp pushed a commit that referenced this pull request Jan 29, 2025
adamleithp pushed a commit that referenced this pull request Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants