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(charts): Update V9 Charts to use DataVizGradientPalette #33323

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

shawn-grant
Copy link
Contributor

@shawn-grant shawn-grant commented Nov 21, 2024

Implement DataVizGradientPalette and rounded corners in v9 charts as default

Modified

  • DonutChart
  • HorizontalBarChart

New Behavior (UI)

DonutChart

HorizontalBarChart

image

Notable Changes:

  • Added DataVizGradientPalette in utilities/gradients.ts
  • Replaced color properties with gradient in types/DataPoint.ts

New Usage

const data: ChartProps = {
  chartData: [
    {
      legend: 'first',
      data: 20000,
      // new `gradient` prop
      gradient: getGradientFromToken(DataVizGradientPalette.gradient1),
    },
    {
      legend: 'second',
      data: 39000,
      // custom gradient
      gradient: [ '#ff88cc', '#db3794' ],
    },
  ],
};

TODO

  • VerticalBarChart

Related

#32008

@shawn-grant shawn-grant marked this pull request as ready for review November 21, 2024 20:27
@shawn-grant shawn-grant requested a review from a team as a code owner November 21, 2024 20:27
@AtishayMsft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

github-actions bot commented Nov 22, 2024

📊 Bundle size report

✅ No changes found

@AtishayMsft
Copy link
Contributor

PR CI is failing.

Copy link

Pull request demo site: URL

@AtishayMsft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

className={classes.root}
style={{ fill: props.color, cursor: href ? 'pointer' : 'default' }}
d={pathData}
style={{ fill: 'transparent', cursor: href ? 'pointer' : 'default' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

why removed the classname?

Copy link
Contributor Author

@shawn-grant shawn-grant Nov 22, 2024

Choose a reason for hiding this comment

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

Moved the classname to the <div> that renders the gradients. The <path> element won't be visible, so the class and its styles aren't needed.
Test also break if both elements have the class.

className={classes.root}
style={{ fill: props.color, cursor: href ? 'pointer' : 'default' }}
d={pathData}
style={{ fill: 'transparent', cursor: href ? 'pointer' : 'default' }}
onFocus={_onFocus.bind(this, props.data!.data, id)}
data-is-focusable={props.activeArc === props.data!.data.legend || props.activeArc === ''}
onMouseOver={_hoverOn.bind(this, props.data!.data)}
onMouseMove={_hoverOn.bind(this, props.data!.data)}
onMouseLeave={_hoverOff}
onBlur={_onBlur}
Copy link
Contributor

Choose a reason for hiding this comment

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

why removed the opacity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to previous comment, the element is to always be transparent, so opacity is unnecessary

@@ -25,7 +25,6 @@ export const Pie: React.FunctionComponent<PieProps> = React.forwardRef<HTMLDivEl
const classes = usePieStyles_unstable(props);
const pieForFocusRing = d3Pie()
.sort(null)
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was showing as an unused eslint directive

}
return { ...item, defaultColor };
})
return { ...item, gradient: item.gradient ?? getNextGradient(index, 0) };
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the gradient colors are undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if item.gradient == undefined it will use the default palette

But if item.gradient == ["", ""] the arc wont be visible. I can update it to also check for this condition

const getById = queryAllByAttribute.bind(null, 'id');
expect(getById(container, /Pie.*?second/i)[0]).toHaveAttribute('opacity', '0.1');
const getByClass = queryAllByAttribute.bind(null, 'class');
expect(getByClass(container, /fui-donut-arc__root/i)[1]).toHaveStyle('opacity: 0.1');
Copy link
Contributor

Choose a reason for hiding this comment

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

dont use class names. These are internal and can change anytime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll switch to id

const DEFAULT_COLORS = Object.values(defaultGradientPalette);
const TOKENS = Object.values(DataVizGradientPalette);

const getThemeSpecificGradient = (gradients: [string, string][], isDarkTheme: boolean): [string, string] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

how can user use a self defined gradient tuple. Could you add an example for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add a new story for charts with custom gradients?

@@ -310,13 +333,13 @@ export const HorizontalBarChart: React.FunctionComponent<HorizontalBarChartProps
x: points.chartData![0].horizontalBarChartdata!.y - datapoint!,
y: points.chartData![0].horizontalBarChartdata!.y,
},
color: tokens.colorBackgroundOverlay,
gradient: getGradientFromToken(DataVizGradientPalette.disabled),
Copy link
Contributor

Choose a reason for hiding this comment

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

use token here as used earlier.

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.

2 participants