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 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e5e79fc
add gradient declarations
shawn-grant Nov 20, 2024
b362372
replace color with gradient in ChartDataPoint and add gradient utilit…
shawn-grant Nov 20, 2024
758f388
refactor: update test-data.ts dataPoints to use gradient property
shawn-grant Nov 20, 2024
09b1252
feat: implement gradient support in DonutChart components
shawn-grant Nov 21, 2024
eef1cf2
test: update DonutChart tests to use gradient instead of color
shawn-grant Nov 21, 2024
0c562b5
feat: add gradient support to HorizontalBarChart and update rendering…
shawn-grant Nov 21, 2024
2eb7d67
refactor: replace color with gradient in HorizontalBarChart test cases
shawn-grant Nov 21, 2024
942d9a3
build: update charts api readme
shawn-grant Nov 21, 2024
d3cc0db
add update-snapshots script to package.json
shawn-grant Nov 21, 2024
5d90039
update tests and test snapshot
shawn-grant Nov 21, 2024
f64134a
Merge branch 'master' into chart-gradients
shawn-grant Nov 21, 2024
d9aa34d
refactor: update all DonutChart stories to use gradient pallete
shawn-grant Nov 21, 2024
40f628c
refactor: update HorizontalBarChart stories to use gradient palette
shawn-grant Nov 21, 2024
355faa6
Merge branch 'master' into chart-gradients
shawn-grant Nov 21, 2024
18988c5
fix HBC test snapshots
shawn-grant Nov 21, 2024
61e5ad7
update change files
shawn-grant Nov 21, 2024
fc6d1c4
update change files
shawn-grant Nov 22, 2024
e10f630
Merge branch 'chart-gradients' of https://github.com/shawn-grant/flue…
shawn-grant Nov 22, 2024
8a3d7bf
update change type to patch
shawn-grant Nov 22, 2024
751e2e0
Merge branch 'master' into chart-gradients
shawn-grant Nov 22, 2024
c58d7af
build: fix formatting issues
shawn-grant Nov 22, 2024
0cc5413
build: fix ci fail (vr-tests-react-components)
shawn-grant Nov 22, 2024
f75812e
add change file
shawn-grant Nov 22, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
shawn-grant marked this conversation as resolved.
Show resolved Hide resolved
"comment": "Introduce gradients and rounded corners to v9 charts",
"packageName": "@fluentui/react-charts-preview",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ export type ChartDataMode = 'default' | 'fraction' | 'percentage';
// @public (undocumented)
export interface ChartDataPoint {
callOutAccessibilityData?: AccessibilityProps;
color?: string;
data?: number;
gradient?: [string, string];
horizontalBarChartdata?: HorizontalDataPoint;
legend?: string;
onClick?: VoidFunction;
Expand Down Expand Up @@ -304,6 +304,36 @@ export interface DataPoint {
y: number;
}

// @public (undocumented)
export const DataVizGradientPalette: {
gradient1: string;
gradient2: string;
gradient3: string;
gradient4: string;
gradient5: string;
gradient6: string;
gradient7: string;
gradient8: string;
gradient9: string;
gradient10: string;
gradient1Ext: string;
gradient2Ext: string;
gradient3Ext: string;
gradient4Ext: string;
gradient5Ext: string;
gradient6Ext: string;
gradient7Ext: string;
gradient8Ext: string;
gradient9Ext: string;
gradient10Ext: string;
success: string;
highSuccess: string;
warning: string;
error: string;
highError: string;
disabled: string;
};

// @public (undocumented)
export const DataVizPalette: {
color1: string;
Expand Down Expand Up @@ -403,9 +433,15 @@ export interface EventsAnnotationProps {
// @public (undocumented)
export const getColorFromToken: (token: string, isDarkTheme?: boolean) => string;

// @public (undocumented)
export const getGradientFromToken: (token: string, isDarkTheme?: boolean) => [string, string];

// @public (undocumented)
export const getNextColor: (index: number, offset?: number, isDarkTheme?: boolean) => string;

// @public (undocumented)
export const getNextGradient: (index: number, offset?: number, isDarkTheme?: boolean) => [string, string];

// @public (undocumented)
export interface GroupedVerticalBarChartData {
name: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"start": "yarn storybook",
"storybook": "yarn --cwd ../stories storybook",
"test": "jest --passWithNoTests",
"update-snapshots": "jest -u",
"type-check": "just-scripts type-check"
},
"syncpack": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ChartDataPoint } from '../index';
import { ArcProps } from './index';
import { format as d3Format } from 'd3-format';
import { formatValueWithSIPrefix, useRtl } from '../../../utilities/index';
import { useId } from '@fluentui/react-utilities';

// Create a Arc within Donut Chart variant which uses these default styles and this styled subcomponent.
/**
Expand All @@ -13,7 +14,7 @@ import { formatValueWithSIPrefix, useRtl } from '../../../utilities/index';
*/
export const Arc: React.FunctionComponent<ArcProps> = React.forwardRef<HTMLDivElement, ArcProps>(
(props, forwardedRef) => {
const arc = d3Arc();
const arc = d3Arc().cornerRadius(5);
const currentRef = React.createRef<SVGPathElement>();
const _isRTL: boolean = useRtl();
const classes = useArcStyles_unstable(props);
Expand Down Expand Up @@ -90,33 +91,58 @@ export const Arc: React.FunctionComponent<ArcProps> = React.forwardRef<HTMLDivEl
//TO DO 'replace' is throwing error
const id = props.uniqText! + props.data!.data.legend!.replace(/\s+/, '') + props.data!.data.data;
const opacity: number = props.activeArc === props.data!.data.legend || props.activeArc === '' ? 1 : 0.1;

const clipId = useId('Arc_clip') + `${props.gradient[0]}_${props.gradient[1]}`;
const gradientFill = `conic-gradient(
from ${props.data?.startAngle}rad,
${props.gradient[0]},
${props.gradient[1]} ${props.data?.endAngle}rad
)`;

const pathData = arc({ ...props.data!, innerRadius: props.innerRadius, outerRadius: props.outerRadius })!;
const focusPathData = arc({ ...props.focusData!, innerRadius: props.innerRadius, outerRadius: props.outerRadius })!;

return (
<g ref={currentRef}>
{!!focusedArcId && focusedArcId === id && (
// TODO innerradius and outerradius were absent
<path
id={id + 'focusRing'}
d={arc({ ...props.focusData!, innerRadius: props.innerRadius, outerRadius: props.outerRadius })!}
d={focusPathData}
className={classes.focusRing}
/>
)}
<path
// TODO innerradius and outerradius were absent
id={id}
d={arc({ ...props.data!, innerRadius: props.innerRadius, outerRadius: props.outerRadius })!}
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.

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

opacity={opacity}
onClick={props.data?.data.onClick}
aria-label={_getAriaLabel()}
role="img"
/>
{/* clipping mask */}
<clipPath id={clipId}>
<path d={pathData} />
</clipPath>
{/* div to attach conic-gradient fill to */}
<foreignObject x="-50%" y="-50%" width="100%" height="100%" clipPath={`url(#${clipId})`}>
<div
className={classes.root}
style={{
width: '100%',
height: '100%',
background: gradientFill,
opacity,
}}
/>
</foreignObject>
{_renderArcLabel(classes.arcLabel)}
</g>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export interface ArcProps {
outerRadius: number;

/**
* Color for the Arc.
* Gradient for the legend in the chart. If not provided, it will fallback on the default color palette.
*/
color: string;
gradient: [string, string];

/**
* Defines the function that is executed upon hovering over the legend
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ function sharedAfterEach() {

// Do this after unmounting the wrapper to make sure if any timers cleaned up on unmount are
// cleaned up in fake timers world
// eslint-disable-next-line @typescript-eslint/no-explicit-any
if ((global.setTimeout as any).mock) {
jest.useRealTimers();
}
Expand Down Expand Up @@ -56,16 +55,16 @@ describe('DonutChart snapShot testing', () => {
});

it('renders DonutChart correctly without color points', () => {
const chartPointColor = pointsDC[0].color;
delete pointsDC[0].color;
const chartPointColor = pointsDC[0].gradient;
delete pointsDC[0].gradient;

let component: any;
rendererAct(() => {
component = renderer.create(<DonutChart data={noColorsChartPoints} />);
});
const tree = component!.toJSON();
expect(tree).toMatchSnapshot();
pointsDC[0].color = chartPointColor;
pointsDC[0].gradient = chartPointColor;
});

it('renders hideLegend correctly', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { DonutChartProps } from './DonutChart.types';
import { useDonutChartStyles_unstable } from './useDonutChartStyles.styles';
import { ChartDataPoint } from '../../DonutChart';
import { convertToLocaleString } from '../../utilities/locale-util';
import { getColorFromToken, getNextColor } from '../../utilities/index';
import { getNextGradient } from '../../utilities/index';
import { Legend, Legends } from '../../index';
import { useId } from '@fluentui/react-utilities';
import { useFocusableGroup } from '@fluentui/react-tabster';
Expand Down Expand Up @@ -76,13 +76,13 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
});
return elevatedData;
}

function _createLegends(chartData: ChartDataPoint[]): JSX.Element {
const legendDataItems = chartData.map((point: ChartDataPoint, index: number) => {
const color: string = point.color!;
// mapping data to the format Legends component needs
const legend: Legend = {
const pointLegend: Legend = {
title: point.legend!,
color,
color: point.gradient![0],
action: () => {
if (selectedLegend === point.legend) {
setSelectedLegend('');
Expand All @@ -98,7 +98,7 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
setActiveLegend('');
},
};
return legend;
return pointLegend;
});
const legends = (
<Legends
Expand All @@ -115,7 +115,7 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
setPopoverOpen(selectedLegend === '' || selectedLegend === data.legend);
setValue(data.data!.toString());
setLegend(data.legend);
setColor(data.color!);
setColor(data.gradient![0]);
setXCalloutValue(data.xAxisCalloutData!);
setYCalloutValue(data.yAxisCalloutData!);
setFocusedArcId(id);
Expand All @@ -128,7 +128,7 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
setPopoverOpen(selectedLegend === '' || selectedLegend === data.legend);
setValue(data.data!.toString());
setLegend(data.legend);
setColor(data.color!);
setColor(data.gradient![0]);
setXCalloutValue(data.xAxisCalloutData!);
setYCalloutValue(data.yAxisCalloutData!);
setDataPointCalloutProps(data);
Expand Down Expand Up @@ -190,17 +190,11 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
);
}

function _addDefaultColors(donutChartDataPoint?: ChartDataPoint[]): ChartDataPoint[] {
function _addDefaultGradients(donutChartDataPoint?: ChartDataPoint[]): ChartDataPoint[] {
return donutChartDataPoint
? donutChartDataPoint.map((item, index) => {
let defaultColor: string;
if (typeof item.color === 'undefined') {
defaultColor = getNextColor(index, 0);
} else {
defaultColor = getColorFromToken(item.color);
}
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

})
: [];
}

Expand Down Expand Up @@ -252,7 +246,7 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
}

const { data, hideLegend = false } = props;
const points = _addDefaultColors(data?.chartData);
const points = _addDefaultGradients(data?.chartData);

const classes = useDonutChartStyles_unstable(props);

Expand All @@ -263,6 +257,7 @@ const DonutChartBase: React.FunctionComponent<DonutChartProps> = React.forwardRe
const chartData = _elevateToMinimums(points.filter((d: ChartDataPoint) => d.data! >= 0));
const valueInsideDonut = _valueInsideDonut(props.valueInsideDonut!, chartData!);
const focusAttributes = useFocusableGroup();

return !_isChartEmpty() ? (
<div
className={classes.root}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ describe('Donut chart interactions', () => {
fireEvent.mouseOver(legend!);

// Assert
const getById = queryAllByAttribute.bind(null, 'id');
expect(getById(container, /Pie.*?second/i)[0]).toHaveAttribute('opacity', '0.1');
expect(getById(container, /Pie.*?third/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');
expect(getByClass(container, /fui-donut-arc__root/i)[2]).toHaveStyle('opacity: 0.1');
});

test('Should select legend on single mouse click on legends', () => {
Expand All @@ -95,8 +95,8 @@ describe('Donut chart interactions', () => {
fireEvent.click(legend!);

// Assert
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');
const firstLegend = screen.queryByText('first')?.closest('button');
expect(firstLegend).toHaveAttribute('aria-selected', 'true');
expect(firstLegend).toHaveAttribute(
Expand All @@ -115,8 +115,8 @@ describe('Donut chart interactions', () => {

//single click on first legend
fireEvent.click(legend!);
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 firstLegend = screen.queryByText('first')?.closest('button');
expect(firstLegend).toHaveAttribute('aria-selected', 'true');
expect(firstLegend).toHaveAttribute(
Expand All @@ -138,13 +138,13 @@ describe('Donut chart interactions', () => {
const legend = screen.queryByText('first');
expect(legend).toBeDefined();
fireEvent.mouseOver(legend!);
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');
fireEvent.mouseOut(legend!);

// Assert
expect(getById(container, /Pie.*?first/i)[0]).toHaveAttribute('opacity', '1');
expect(getById(container, /Pie.*?second/i)[0]).toHaveAttribute('opacity', '1');
expect(getByClass(container, /fui-donut-arc__root/i)[0]).toHaveStyle('opacity: 1');
expect(getByClass(container, /fui-donut-arc__root/i)[1]).toHaveStyle('opacity: 1');
});

test('Should display correct callout data on mouse move', async () => {
Expand Down
Loading
Loading