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

feat(trends): allow customization of result dataset colors #26238

Closed
wants to merge 46 commits into from

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Nov 18, 2024

Problem

Users want to be able to customize the colors in insights. Most often this is because multiple insights on the same dashboard should have the same color for the same values. See #23093 and #17593.

Changes

This PR allows customizing the colors of (most) trend insight charts.

  • This feature is behind the feature flag insight-colors for now. I'd love to have some feedback first - see also the follow-up points further below. Further, we need to decide wether this feature should be available on a free plan. I'd argue "yes" for the main feature (see use case in problem description), but maybe "no" for things like project wide custom color themes.
  • Note that the table below insights now looks slightly different, also when the feature flag is turned of. This is so that the trend and funnel insight look more similar.
  • There's now an option for wether result dataset should be assigned by their position (e.g. highest value) or by their value (e.g. breakdown "Chrome" for the first series"):
    Screenshot 2024-11-19 at 14 17 40
  • The modal for the customizations can be opened from the insights table:
    Screenshot 2024-11-19 at 14 17 58
  • It looks like this:
    Screenshot 2024-11-19 at 14 18 07

Open questions

  • Should colors for the "compare to previous" option change together or should the user be able to pick them separately?
  • Schema: Should we make resultCustomizations part of trendsFilter/funnelsFilter/etc.?
  • Pricing / plan availability

Potential follow-up work / ideas

  • Obviously, make this feature work for other insight types e.g. funnels or lifecycle.
  • Add support for customizing the default theme on a team level
  • Add support for adding other themes on a team level
  • Add support for customizing the default theme on a user level (color blindness)
  • Add support for customizing the theme on an insight level
  • Add support for arbitrary hex colors, instead of needing to pick from the color palette. We already have this for SQL insights. How to handle light/dark mode?

How did you test this code?

Tried locally

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Nov 19, 2024

Size Change: +32 B (0%)

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.11 MB +32 B (0%)

compressed-size-action

@thmsobrmlr thmsobrmlr marked this pull request as ready for review November 19, 2024 15:16
@thmsobrmlr thmsobrmlr requested a review from a team November 19, 2024 15:16
@anirudhpillai
Copy link
Contributor

These changes look great, you've built a very slick UX! 😍

Probably just me, but for the colour picker, maybe instead of the gear icon, you can show the colour itself? Users might not realise right away that the gear lets them change colours. (Though if you intend for the gear to control more things other than colour in the future, can leave as is, but maybe add a tooltip to the gear?)

image image

Also like everything on the insight page, when you change series colour without going into edit mode, and then add the insight to a dashboard/refresh insight, the series colour isn't preserved. Maybe we should show a warning to the user that the colour change won't be preserved or only let them edit the series colour in edit mode?

colour_not_persisted.mov

@aspicer
Copy link
Contributor

aspicer commented Nov 21, 2024

With an insight on "by value"
set a color, change to "by position," and then go to select a color. I get this error:
image

@aspicer
Copy link
Contributor

aspicer commented Nov 21, 2024

This is great! I think users are going to love this.

  • I think maybe a little more clarity is needed to describe these two different customization types in the tooltip.
    • I feel like "by name" and "by rank" might be clearer options for the names
    • "by rank" makes it clearer that it is sorted by output than "by position," which reads to me more like "series 1, series 2, series 3"
    • feel less strongly about "by name" but "by value" makes me think of the total sum (as in the value in a key value pair).
  • Maybe let's make the header "color customization by" instead of "result customization by" until we add other types of customization
  • Is it possible to add tooltip color names to the color picker? I'm color blind, so I rely on color names and positioning a lot when interacting with and picking things like color.
  • I wonder if it would make sense to have a "color modal" where we display a subset of the result table and let the person pick colors for all of the series at once (and see them all together to avoid picking the same color multiple times), instead of having it pop up one by one. Not necessary to launch, just thinking about future usability. In "by rank" mode, this picker could be independent of results and just exist (could be used globally for setting team colors also).

Copy link
Contributor

@aspicer aspicer left a comment

Choose a reason for hiding this comment

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

Left some comments and a bug report, but we could release to the posthog team behind a flag whenever.

@Hronom
Copy link

Hronom commented Dec 3, 2024

When it will be released?

@posthog-bot posthog-bot removed the stale label Dec 4, 2024
@thmsobrmlr thmsobrmlr removed the stale label Dec 12, 2024
@PostHog PostHog deleted a comment from posthog-bot Dec 12, 2024
@PostHog PostHog deleted a comment from posthog-bot Dec 12, 2024
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@thmsobrmlr
Copy link
Contributor Author

Rolled into #26862

@Hronom sometime this week

@thmsobrmlr thmsobrmlr closed this Dec 16, 2024
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