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

frontend: add and use charts colors #2926

Merged
merged 10 commits into from
Feb 26, 2024
Merged

frontend: add and use charts colors #2926

merged 10 commits into from
Feb 26, 2024

Conversation

jecr
Copy link
Contributor

@jecr jecr commented Feb 8, 2024

This PR adds the color palette to be used in charts and updates current usages with references to the theme. A new property was added to the theme object: chartColors, to hold an static array of colors.

Added default colors to the theme.colors object for the Pie and LinearTimeline components, TimeseriesChart does not use default colors, but their stories now use colors from theme.

Changed components:

Linear Timeline

before
image

after
image

Custom styled

before
image

after
image

Pie Chart

Chart now uses colors from theme
Changed labels to get colors from palette

before
image

after
image

TimeseriesChart

before
image

after
image

before
image

after
image

Testing Performed

manual, unit tests

@jecr jecr marked this pull request as ready for review February 8, 2024 18:22
@jecr jecr requested a review from a team as a code owner February 8, 2024 18:22
Copy link
Contributor

@jdslaugh jdslaugh left a comment

Choose a reason for hiding this comment

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

I think this will need to be handled slightly differently but good initial work overall!

@@ -173,6 +173,23 @@ export const DARK_COLORS: ClutchColors = {
},
};

export const clutchChartColors: string[] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wonder if we should be a little more descriptive with how we approach this. If we're to support multiple themes then this will likely not work well as it is very static. Also it should probably be a child of colors so it will inherit the variant that is passed down. Then also, what would we do if we wanted to hold different colors based on the chart/type etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted the charts colors prop to be under colors, so it'll look like

interface ChartColors {
  common: string[];
}

export interface ClutchColors {
  neutral: Color;
  blue: Color;
  green: Color;
  amber: Color;
  red: Color;
  charts: ChartColors;
}

commons felt right since the colors collection could be used by multiple charts. Having charts, with a different structure, within ClutchColors seems a bit odd though, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

So one structure we could utilize is:

charts: {
  data: [],
  pie: {},
  graph: {},
}

This way we have the common data colors that all charts will likely use and then in the future we have the capability of subbing the type of charts with further data.

common feels like it could work but the word intones that these are all common colors shared across and therefore would likely have children of its own, such as common.data.

So I think we can utilize what I mentioned above as:

charts: {
  common: {
    data: []
  },
  pie: {},
  graph: {},
}

This way it allows us to add further common variables in the future if we need to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly I don't know 'what' we'll need in the future, so this is just preparation to make sure we can support it :)

@jecr jecr requested a review from jdslaugh February 15, 2024 21:11
Copy link

This PR has been marked as stale after 7 or more days of inactivity. Please have a maintainer add the on hold label if this PR should remain open. If there is no further activity or the on hold label is not added, this PR will be closed in 3 days.

@github-actions github-actions bot added the stale Issue hasn't had activity in awhile label Feb 22, 2024
@jecr jecr removed the stale Issue hasn't had activity in awhile label Feb 22, 2024
Copy link
Contributor

@jdslaugh jdslaugh left a comment

Choose a reason for hiding this comment

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

Looks good

@jecr jecr merged commit 019fbeb into main Feb 26, 2024
8 checks passed
@jecr jecr deleted the add-charts-colors branch February 26, 2024 16:59
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