-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: adding white label event capture for sharing #27140
base: master
Are you sure you want to change the base?
Conversation
6510357
to
53e5a50
Compare
Size Change: +197 B (+0.02%) Total Size: 1.11 MB ℹ️ View Unchanged
|
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.
This sharing logic is used across multiple resource types (dashboard, insight, recordings) and it looks like we're already triggering events for dashboard sharing so it might be best to follow that pattern and expand for insights and recordings.
See setIsEnabled
listener for the dashboard trigger and SharingLogicProps
for the different id props passed in.
aa57343
to
7ec7db3
Compare
Based on the feedback abo e,
I've customized the kea action setConfigValue to log when the user toggles the whitelabel button. I didn't think loaders were necessary here, as this action doesn't trigger any API calls and is handled entirely on the frontend. I'll add the changes for insights and surveys next. |
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.
Two suggestions but if those are addressed this looks good.
@@ -174,9 +174,9 @@ export function SharingModalContent({ | |||
</div> | |||
} | |||
onChange={() => | |||
guardAvailableFeature(AvailableFeature.WHITE_LABELLING, () => | |||
onChange(!value) |
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.
We should still call onChange
here so it we ever save this to the API the form state is updated.
@@ -791,6 +792,9 @@ export const eventUsageLogic = kea<eventUsageLogicType>([ | |||
reportDashboardShareToggled: async ({ isShared }) => { | |||
posthog.capture(`dashboard share toggled`, { is_shared: isShared }) | |||
}, | |||
reportDashboardWhitelabelToggled: async ({ isWhitelabelToggled }) => { | |||
posthog.capture(`dashboard whitelabel toggled`, { is_whitelabel_toggled: isWhitelabelToggled }) |
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.
I find this attribute a bit confusing is_whitelabelled
might be better.
Problem
In this PR, I'm adding whitelabel capture event for sharing dashboards
In the following PR's,
I will add changes to
Changes
The event capture happens in the frontend since this feature is purely built in the frontend.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Cloud: Yes
How did you test this code?
Shared a dashboards, and toggled the whitelabel button. Verified logs showed up: