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

Apply mantine color palette to cartesian chart labels #42148

Merged
merged 8 commits into from
May 2, 2024

Conversation

heypoom
Copy link
Contributor

@heypoom heypoom commented May 2, 2024

Closes #41949

Description

Cartesian charts currently get their color from the color function from lib/colors/palette.ts, but these colors are hard-coded and not overrideable by users. This PR adds the use-palette hook that extracts a subset of color palettes from a Mantine theme, and apply the palette into the cartesian chart.

How to verify

  1. Go to the basic-theming branch in the demo app
  2. Go to the /admin/internal/kitchen-sink page
  3. Verify that the theme color in AppProvider is set to a custom value, e.g. text-dark is hotpink:
  colors: {
    brand: colorTuple("hotpink"),
    "text-dark": colorTuple("hotpink"),
    "text-light": colorTuple("hotpink"),
  },
  1. The 3 chart labels should now be in hotpink, instead of black.

Screenshot

Labels used to use the hard-coded text-dark color, but now it becomes hotpink as defined above.

CleanShot 2567-05-02 at 19 01 08@2x

Checklist

  • Tests have been added/updated to cover changes in this PR

@heypoom heypoom added the no-backport Do not backport this PR to any branch label May 2, 2024
@heypoom heypoom marked this pull request as ready for review May 2, 2024 12:26
@heypoom heypoom requested a review from a team May 2, 2024 12:26
@metabase-bot metabase-bot bot added the visual Run Percy visual testing label May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff f852b48...9e4b51e.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/components/Visualization/Visualization-themed.unit.spec.tsx
frontend/src/metabase/visualizations/visualizations/CartesianChart/use-models-and-option.ts

Copy link

replay-io bot commented May 2, 2024

Status Complete ↗︎
Commit 9e4b51e
Results
⚠️ 9 Flaky
2442 Passed

frontend/src/metabase/hooks/use-palette.ts Outdated Show resolved Hide resolved
@@ -49,12 +52,12 @@ export function useModelsAndOption({

const renderingContext: RenderingContext = useMemo(
() => ({
getColor: color,
getColor: name => color(name, palette),
Copy link
Member

Choose a reason for hiding this comment

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

passing colors we want to override here looks good. I kind of remember we did this when we worked with static viz to make user appearance colors works.

@heypoom heypoom requested a review from WiNloSt May 2, 2024 13:09
@heypoom heypoom requested a review from a team May 2, 2024 13:11
@heypoom heypoom enabled auto-merge (squash) May 2, 2024 15:29
@heypoom heypoom merged commit ca1dbda into master May 2, 2024
111 of 112 checks passed
@heypoom heypoom deleted the cartesian-chart-label-theming branch May 2, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/Embedding visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cartesian charts labels should inherit color from Mantine theme in React embedding
2 participants