-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
base: master
Are you sure you want to change the base?
Conversation
change/@fluentui-react-charts-preview-d9ff82b5-6274-429e-8f9d-bd400e647d4b.json
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
📊 Bundle size report✅ No changes found |
PR CI is failing. |
Pull request demo site: URL |
/azp run |
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' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removed the classname?
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removed the opacity
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this line
There was a problem hiding this comment.
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) }; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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] => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
Implement DataVizGradientPalette and rounded corners in v9 charts as default
Modified
New Behavior (UI)
DonutChart
HorizontalBarChart
Notable Changes:
utilities/gradients.ts
color
properties withgradient
intypes/DataPoint.ts
New Usage
TODO
Related
#32008