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

chore: add device type filtering on surveys #28109

Merged
merged 8 commits into from
Feb 5, 2025

Conversation

lucasheriques
Copy link
Contributor

@lucasheriques lucasheriques commented Jan 30, 2025

Problem

PR for SDK logic: PostHog/posthog-js#1698

Closes #24495

Changes

CleanShot 2025-02-04 at 15 50 35@2x

adds this a new display condition for Surveys: Device Types

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Tested on local posthog instance if a Survey set to display on specific device types was only shown on those devices.

@lucasheriques lucasheriques requested a review from a team January 30, 2025 18:11
@lucasheriques lucasheriques self-assigned this Jan 30, 2025
Copy link
Contributor

github-actions bot commented Jan 30, 2025

Size Change: 0 B

Total Size: 1.17 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.17 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@marandaneto marandaneto requested a review from ioannisj January 30, 2025 22:50
@lucasheriques lucasheriques marked this pull request as ready for review February 4, 2025 18:16
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Here's my summary of the key changes in this PR:

Adds device type filtering capability to PostHog surveys, allowing targeting based on device types (Mobile, Tablet, Desktop).

  • Renamed SurveyUrlMatchType to SurveyMatchType in types.ts to support both URL and device type matching conditions
  • Added new fields deviceTypes and deviceTypesMatchType to Survey interface conditions object
  • Implemented device type display condition UI in SurveyDisplaySummary component with matching type selector
  • Added inputClassName prop to PropertyValue component to support device type filtering styles
  • Updated survey logic to handle validation and processing of device type matching conditions

The changes enable more granular survey targeting while maintaining backward compatibility with existing URL-based targeting.

7 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

@marandaneto
Copy link
Member

@lucasheriques are you planning to add tests?

@lucasheriques
Copy link
Contributor Author

@marandaneto the logic tests don't really cover any display conditions tests for now - they only cover branching and different survey types (open vs multiple question etc). so no plan to add new tests for this case now on my end

@lucasheriques lucasheriques enabled auto-merge (squash) February 4, 2025 23:23
Comment on lines +1900 to +1908
export function getPropertyKey(value: string, type: TaxonomicFilterGroupType): string {
// Find the key by looking through the mapping
const group = CORE_FILTER_DEFINITIONS_BY_GROUP[type]
if (group) {
const foundKey = Object.entries(group).find(([_, def]) => (def as any).label === value || _ === value)?.[0]
return foundKey || value
}
return value
}
Copy link
Member

Choose a reason for hiding this comment

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

@pauldambra do you know any other way to use the labels/keys from taxonomy instead of duplicating everywhere? I feel like this should be done in a lot of places already so I wonder if this method is necessary.
Not a blocker since it is approved and it works :)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe getFilterLabel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keys are different than labels here. Key I'm treating as the $device_type, and label would be Device Type.

I didn't find a way in the codebase where this existed already, which is why I created this function 🤔 but if @pauldambra i'd be happy to fix it!

@lucasheriques lucasheriques merged commit 75c9767 into master Feb 5, 2025
103 checks passed
@lucasheriques lucasheriques deleted the chore/add-device-type-filtering-on-surveys branch February 5, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surveys: Allow targeting by device type
3 participants