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

fix: friendlier label names for feature flags property targetting #28301

Merged

Conversation

lucasheriques
Copy link
Contributor

Problem

adds the friendlier label name to the feature flag conditions match, so users don't have to set a "Latest Current URL" and then see just a $current_url key which doesn't carry a lot of meaning.

Changes

Before After
CleanShot 2025-02-04 at 17 43 06@2x image

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

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

Yes

How did you test this code?

Verified locally, no logic changes

@lucasheriques lucasheriques requested a review from a team February 4, 2025 20:44
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

Enhanced feature flag property targeting UI by adding human-readable labels for property keys in the feature flag conditions display.

  • Added getFilterLabel function call in frontend/src/scenes/feature-flags/FeatureFlagReleaseConditions.tsx to show descriptive labels (e.g. "Latest Current URL" instead of "$current_url")
  • Modified read-only property display to show both friendly label and technical key for better clarity
  • Improved UX by making property targeting conditions more understandable for non-technical users

1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +240 to +246
{property?.type !== 'cohort' &&
getFilterLabel(
property.key,
property.type === 'person'
? TaxonomicFilterGroupType.PersonProperties
: TaxonomicFilterGroupType.EventProperties
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The property type check should also handle group properties, not just person and event properties

Copy link
Member

Choose a reason for hiding this comment

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

why do we filter type !== 'cohort'?
should we also consider group properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for cohort, I don't think we need the helper table:

CleanShot 2025-02-05 at 17 05 53@2x

also for group properties, I thought we only had person properties and cohorts for filtering. Looked on the feature flags page too:

CleanShot 2025-02-05 at 17 06 38@2x

Copy link
Contributor

github-actions bot commented Feb 4, 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

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

  • chromium: 0 added, 1 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 a team February 5, 2025 09:06
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 1 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.

@lucasheriques lucasheriques merged commit f559586 into master Feb 5, 2025
103 checks passed
@lucasheriques lucasheriques deleted the fix/more-friendly-label-names-for-property-targetting branch February 5, 2025 22:18
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.

3 participants