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

refactor: cleanup colors #1320

Closed
wants to merge 8 commits into from

Conversation

markov00
Copy link
Member

Summary

During the heatmap legend fixes I've noticed that we have multiple places where some standard colors are allocated.
I've moved the Color type + some static colors (red, white, black, transparent) into the common/color.ts file.
This is not a complete refactoring but the first part of the initiative to cleanup the folders the typings.

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)

@markov00 markov00 added :colors colors related issue skip-newsletter :all Applies to all chart types labels Aug 20, 2021
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM, I would suggest a slightest simplified API. See comments.

packages/charts/src/common/color.ts Outdated Show resolved Hide resolved
storybook/stories/goal/10_band_in_band.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM, good to merge once it passes and conflicts are resolved

Switching to some color object (or fake enum) would be sensible if we expect significant growth to the count of these constants, or a COLOR_ prefix at least. With the current handful of colors, I don't feel strongly either way. Worth doing a last minute grep for all string occurrences 'black' or even without quotes to ferret out template string occurrences or similar

@markov00
Copy link
Member Author

markov00 commented Sep 1, 2021

just an update: I've found that transparent and rgba(0,0,0,0) are not treated the same way in the partition chart. I'm currently investigating to fix this

@nickofthyme nickofthyme force-pushed the master branch 7 times, most recently from 7148acf to 604b153 Compare September 13, 2021 04:06
@markov00
Copy link
Member Author

close in favour of #1397

@markov00 markov00 closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types code quality :colors colors related issue skip-newsletter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants