-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: remove geolocation from session replay onboarding #22341
Conversation
Size Change: 0 B Total Size: 1.03 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.
Looks fine, just not sure if it makes more sense to be an allowlist instead of a denylist
} | ||
type PluginContentMapping = Record<string, PluginContent> | ||
const pluginContentMapping: PluginContentMapping = { | ||
GeoIP: { | ||
title: 'Capture location information', | ||
description: | ||
'Enrich PostHog events and persons with IP location data. This is useful for understanding where your users are coming from. This setting can be found under the data pipelines apps.', | ||
productOnboardingDenyList: [ProductKey.SESSION_REPLAY], |
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 seems backwards to me, I feel like it should maybe be an allowlist.
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.
Do we not want the setting on all pages except those listed?
This felt clearer because we're saying "don't show only this one" over "do show on these other ones" and then you have to figure out how many it won't be shown on
No strong feeling here, since you all know this area better than me :)
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.
will merge it since it's green, @raquelmsmith happy to change to an allowlist if you have strong opinions.
but I agree with @pauldambra here, it's easier to add to a deny list rather than to an allowlist for all the new features (looking at less maintenance)
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.
Sorry I forgot about this!
I think for this specific instance the denylist makes sense. But as we move forward with other products that aren't so tied to product analytics (eg data warehouse) we'll have to remember to add those to the denylist. An allowlist is a better construct imo. But, we can change it in the future if / when it starts to get annoying. Thanks for getting this in!
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
I ran through onboarding and mentioning geo-location during session replay onboarding was confusing
There's not geo-location for session replay, it only applies to event ingestion. So this is setting incorrect expectations for folk who are only going to use session replay
## other onboarding pages still list it
## session replay onboarding page does not