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(data-warehouse): Select color of series on a query visualization #25909

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

rossgray
Copy link
Contributor

Problem

At the moment it is not possible to change the color of a series on a query visualization.

Changes

UX

Adds a color picker (the same one as for conditional formatting, thanks Tom!) to the series display options tab. See screen recording below.

Screen.Recording.2024-10-30.at.17.21.54.mov

Code

  • I have have refactored the ColorPickerButton into its own file since it is now used in 2 different places (I also renamed it ColourPickerButton -> ColorPickerButton since we use the American spelling in most places and thought it best to be consistent).
  • I also added a colorChoices prop so that for the series color we can pass in the set of colors currently used for series (this is different from the color palette used for conditional formatting)

Limitations

  • It is not ideal having to click off of the display settings before seeing the visualization update, however it is not straightforward to change this behaviour so decided to leave it for now.
  • This is already an issue but there is not much space on the select for the series label
    image
  • In Metabase you can change the color of a series directly by clicking on the color glyph directly, without having to open up the settings tab (see below).
    image
    I thought about doing the same - I think it would require us to move the colour glyph out of the select to a separate button on the left (see below). The only thing is that there is then even less space to display the label, unless we change the layout a bit, so not sure whether to proceed with this or not?
    image

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

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

I presume so.

How did you test this code?

Local testing

@rossgray rossgray requested a review from a team October 30, 2024 17:36
Copy link
Member

@Gilbert09 Gilbert09 left a comment

Choose a reason for hiding this comment

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

Just the one change, then this is good - top stuff! 🚀

@@ -668,6 +668,7 @@ export interface ChartSettingsFormatting {
}

export interface ChartSettingsDisplay {
color?: string
Copy link
Member

Choose a reason for hiding this comment

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

There should be a backend equivalent for this - generate it with pnpm schema:build command. Without it, I think saving an insight with a color will get rejected by the backend (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, I hadn't tested saving an insight. Will try what you said 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that should be fixed now

@rossgray rossgray changed the title Ross/query viz color feat(data-warehouse): Select color of series on a query visualization Oct 31, 2024
@rossgray rossgray force-pushed the ross/query-viz-color branch from 1f89827 to 326d6ae Compare October 31, 2024 10:09
Copy link
Contributor

github-actions bot commented Oct 31, 2024

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 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 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@rossgray rossgray enabled auto-merge (squash) October 31, 2024 10:28
@rossgray rossgray merged commit 44f28c3 into master Oct 31, 2024
96 checks passed
@rossgray rossgray deleted the ross/query-viz-color branch October 31, 2024 11:23
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