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: custom chart colors support #836

Merged
merged 13 commits into from
Dec 2, 2023
Merged

feat: custom chart colors support #836

merged 13 commits into from
Dec 2, 2023

Conversation

adamhenson
Copy link
Contributor

@adamhenson adamhenson commented Dec 2, 2023

Description

As described in #565, chart colors are limited to base colors typed as Color.

For example, this chart will render line colors as expected.

<LineChart {...props} colors={["emerald", "indigo"]} />

But, this will not work.

<LineChart {...props} colors={["#32a852", "custom-color-100"]} />

This limitation seems to be due to the complexity in which classNames are generated under the hood (dynamically at run time and statically at build time via Tailwind theme safelist). See getColorClassNames.

Changes

This change introduces a new optional customChartColors prop on chart components to support custom colors (including hex or custom theme colors) on the following components.

  • AreaChart
  • BarChart
  • DonutChart
  • LineChart
  • ScatterChart
  • SparkAreaChart
  • SparkBarChart
  • SparkLineChart

Implementation example below.

<LineChart {...props} customChartColors={["#32a852", "custom-color-100"]} />

The caveat here is that such implementation would require an update to the Tailwind theme safelist to ensure Tailwind generates classNames at build time to mirror classNames calculated at run time in functions like getColorClassNames. This caveat is somewhat pre-existing, with the reliance on safelist. My thought was that we could add a new section to the theming docs if using customChartColors. Please let me know if I can help here.

Example safelist to support the above implementation:

safelist: [
  // ..
  // ..
  // custom colors
  ...["[#32a852]", "[#fcba03]"].flatMap((customColor) => [
    `bg-${customColor}`,
    `border-${customColor}`,
    `hover:bg-${customColor}`,
    `hover:border-${customColor}`,
    `hover:text-${customColor}`,
    `fill-${customColor}`,
    `ring-${customColor}`,
    `stroke-${customColor}`,
    `text-${customColor}`,
    `ui-selected:bg-${customColor}]`,
    `ui-selected:border-${customColor}]`,
    `ui-selected:text-${customColor}`,
  ]),
],

Related issue(s)

What kind of change does this PR introduce?

  • Bug fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

How has This been tested?

Storybook stories have been added for all components with the new customChartColors prop.

The PR fulfills these requirements:

  • It's submitted to the main branch
  • When resolving a specific issue, it's referenced in the related issue section above
  • My change requires a change to the documentation. (Managed by Tremor Team)
  • I have added tests to cover my changes
  • Check the "Allow edits from maintainers" option while creating your PR.
  • Add refs #XXX or fixes #XXX to the related issue section if your PR refers to or fixes an issue.
  • By contributing to Tremor, you confirm that you have read and agreed to Tremor's CONTRIBUTING.md guideline. You also agree that your contributions will be licensed under the Apache License 2.0 license.

Copy link

vercel bot commented Dec 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tremor-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2023 2:18pm

@severinlandolt severinlandolt self-requested a review December 2, 2023 13:52
@severinlandolt severinlandolt added Priority: Medium This issue/feature is of medium priority PR: Under Consideration Has potentially wider implications which are being discussed labels Dec 2, 2023
@severinlandolt severinlandolt linked an issue Dec 2, 2023 that may be closed by this pull request
@severinlandolt
Copy link
Member

severinlandolt commented Dec 2, 2023

Oh wow, what an amazing PR! 🤩 I am a big fan of your implementation details and thank you for the thorough description.

My thoughts on safelist: Not a big concern in my view, given the usefulness of the feature. (Note: #484 also facilitates an adjustment to the config, should be released together)

Will take a look later today and thank you very much for your work!

@severinlandolt severinlandolt changed the base branch from main to beta December 2, 2023 14:39
@adamhenson
Copy link
Contributor Author

Thanks so much @severinlandolt 🙏 and I agree about safelist. Also thanks for fixing the lint issue!

@severinlandolt severinlandolt merged commit 9231a27 into tremorlabs:beta Dec 2, 2023
10 checks passed
Copy link

github-actions bot commented Dec 2, 2023

🎉 This PR is included in version 3.12.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@severinlandolt
Copy link
Member

@adamhenson
Copy link
Contributor Author

Awesome, thanks so much for the quick help on this one @severinlandolt. Appreciate this project 🙏

Copy link

github-actions bot commented Dec 2, 2023

🎉 This PR is included in version 3.12.0-beta-package-updates.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@severinlandolt
Copy link
Member

Hey @adamhenson, we were discussing the following:

Integrate the customChartColors logic into the color prop.
→ Include a hint in the documentation indicating that the color prop on the charts also supports custom values.

What do you think? And would you have the time to create another PR with those changes?

@adamhenson
Copy link
Contributor Author

Hi @severinlandolt - yes, that makes sense to me. I was thinking about that route too as eventually I think I'd want to be able to use custom colors in more than just these charts. My only concern would be around programmatically determining if a color/s prop has a custom color or not. In my change, I was able to hook into color className generation everywhere by knowing if we're dealing with a custom color via the explicit customChartColors prop.

I can definitely take a deeper dive and see what I can do. I'm pretty busy this week, but if I don't get to it this week, I can probably spend some time on this next weekend.

@severinlandolt
Copy link
Member

That would be great @adamhenson! Feel free to branch off from beta it's the most up to date branch.

@adamhenson
Copy link
Contributor Author

Hey @severinlandolt - if I can pull this off, I'm assuming I should remove customChartColors in the same PR - correct?

@severinlandolt
Copy link
Member

@adamhenson Yes, that would be awesome!

@adamhenson
Copy link
Contributor Author

Okay, I believe #843 is what you're after @severinlandolt to "Integrate the customChartColors logic into the color prop".

Copy link

🎉 This PR is included in version 3.12.0-beta-customcolors.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Mxs2019
Copy link

Mxs2019 commented Mar 26, 2024

Was this ever released? I need the ability to use custom hex colors at run time since they aren't known at generator time. Is there any way to do that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Under Consideration Has potentially wider implications which are being discussed Priority: Medium This issue/feature is of medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Support for custom hex colors
3 participants