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

Multivariate flags UI #5725

Merged
merged 13 commits into from
Aug 26, 2021
Merged

Multivariate flags UI #5725

merged 13 commits into from
Aug 26, 2021

Conversation

samwinslow
Copy link
Contributor

@samwinslow samwinslow commented Aug 24, 2021

Changes

Contributes to #5440. Adds a UI in PostHog for configuring multivariate feature flags.

Notes

  • This feature itself is feature-flagged behind 5440-multivariate-support
  • Client-side library support (i.e. our JS and Python consumers) is not yet available, so this is just a UI for updating feature flag records in the database.
  • This will be tested internally for Growth projects before any kind of public release

The feature flag rollout page:
Screen Shot 2021-08-24 at 1 13 24 PM

Validations:
Screen Shot 2021-08-24 at 1 14 14 PM
Screen Shot 2021-08-24 at 1 15 01 PM

Distribute evenly (handling remainders from integer rounding):
Screen Cast 2021-08-24 at 1 16 03 PM

Data loss warning if switching from variants to Boolean (default) behavior:
Screen Cast 2021-08-24 at 1 18 36 PM

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • New/changed UI is decent on smartphones (viewport width around 360px)

@timgl timgl temporarily deployed to posthog-pr-5725 August 24, 2021 17:21 Inactive
@timgl timgl temporarily deployed to posthog-pr-5725 August 25, 2021 14:26 Inactive
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Hey, apologies this took long to review, but I think this looks good! Approved, but with two comments. See if they make sense to address.

  1. As one small change, I'd still add the PR or issue ID inside the feature flag, like the other ones have.
  2. As one bigger change, it would be cool if you would limit the multivariate sliders so that none can be dragged such that the total is over 100%.

@timgl timgl temporarily deployed to posthog-pr-5725 August 26, 2021 14:36 Inactive
@samwinslow
Copy link
Contributor Author

Thank you @mariusandra! Regarding the sliders — TL;DR I had considered doing that, but felt it would arbitrarily limit the user.

Consider some example percentages are [50%, 50%]
User wants to adjust them to [70%, 30%]

Preventing the total from exceeding 100, even temporarily, would force the user to decrease the rollout of one to 30% before increasing the other to 70% (forcing an order of operations).

But this intermediate value of (50 + 30) = 80% is just as incorrect as an intermediate value of (70 + 50) = 120% as the final allocation must sum to exactly 100.

@samwinslow samwinslow enabled auto-merge (squash) August 26, 2021 14:44
@samwinslow samwinslow merged commit 5b589c5 into master Aug 26, 2021
@samwinslow samwinslow deleted the multivariate-flags-ui branch August 26, 2021 14:59
@mariusandra
Copy link
Collaborator

Makes sense and agreed, that proposal is also not ideal, and ultimately not better than what's in the PR.

cc @clarkus for thoughts.

@mariusandra mariusandra mentioned this pull request Aug 27, 2021
6 tasks
@paolodamico
Copy link
Contributor

Just wanted to provide some quick late feedback here. I think we can make the "Boolean" & "String variant" labels clearer for a less technical audience.

@clarkus
Copy link
Contributor

clarkus commented Aug 30, 2021

Circling back here to review - I think this is a good v1 especially with the notes @mariusandra and @paolodamico added. The boolean and string options could have some inline tooltips to explain them to less technical audiences or we could change the nomenclature to "true / false" or "text". Whatever we do here should probably align with any data types we expose in the rest of the product.

Sliders are a really challenging component to use for users and a challenging one to validate for engineers. It really comes down to implementation and how accurate a user can be with their input. The input fields here that capture the value are a good idea - you can skip sliders altogether and just type in your distribution.

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.

5 participants