Shopify / polaris-viz

A collection of React and React native components that compose Shopify's data visualization system
https://polaris-viz.shopify.com
Other
326 stars 24 forks source link

Make sure all styles of the chart come from the Theme #1021

Closed krystalcampioni closed 2 years ago

krystalcampioni commented 2 years ago

Context

All Polaris Viz Charts should fetch styles from the Theme definition

TODO

Designs

@adamperron @aieaee if you have the time could you work on a dark mode mock for the chart? Most of the colors we can get from the theme definition, but I don't think we have a dark mode equivalent of that gray area between bars

clairedeboer commented 2 years ago

TODO 2: @krystalcampioni @juwanpetty We're using roundedBorder, should we be using hasRoundedCorners instead?

clairedeboer commented 2 years ago

TODO 1: @krystalcampioni @envex We have useTheme coming from @shopify/polaris-viz-core in the Label which works fine, but useTheme is coming from ../../hooksin the Chart and Funnel Chart and when I change it to @shopify/polaris-viz-core I get a reference error saying useTheme is not defined. Any ideas?

clairedeboer commented 2 years ago

TODO 3: @juwanpetty @krystalcampioni We have the bar color coming from MASK_HIGHLIGHT_COLOR

In looking at using the seriesColors.single I see it only has 3 elements and we have four bars. Would we be using seriesColors.upToFour instead?

envex commented 2 years ago

@clairedeboer I just tried on the feature/FunnelChart branch and it seems to work, make sure you're importing on it's own and not with import type.

// This way is correct
import type {
  DataSeries,
  XAxisOptions,
  YAxisOptions,
} from '@shopify/polaris-viz-core';
import {useTheme} from '@shopify/polaris-viz-core';

// This will error
import type {
  DataSeries,
  useTheme,
  XAxisOptions,
  YAxisOptions,
} from '@shopify/polaris-viz-core';

When I initially checked, vscode auto imported it into the import type {} group.

clairedeboer commented 2 years ago

@envex Yep that was definitely the problem, works now. Thank you thank you!

krystalcampioni commented 2 years ago

@clairedeboer @juwanpetty the bars should use seriesColors.single because the chart will always display a single series, and not many, regardless of how many bars it displays.

I also noticed that the feature/FunnelChart branch has some conflicts with main. Make sure you get the latest changes so you don't work on top of stale code 🫂

clairedeboer commented 2 years ago

@krystalcampioni Good call on getting the latest changes, thank you!

Also, I'm still confused on this.single is an array of 3 items and I don't see how it fits into our bar. How do we know which one to choose? https://github.com/Shopify/polaris-viz/blob/main/packages/polaris-viz-core/src/constants.ts#L96

envex commented 2 years ago

@clairedeboer NEUTRAL_SINGLE_GRADIENT is a single gradient, so the array is an array of stops.