-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(react-charting): Heatmap text color based on Contrast Ratio #33659
base: master
Are you sure you want to change the base?
Changes from all commits
353021a
949d452
e2a787d
1dc7207
5465301
0e4c0f2
0768fb1
0f6c5aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,7 @@ | ||||||||||||||
{ | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🕵🏾♀️ visual regressions to review in the fluentuiv9 Visual Regression ReportAvatar Converged 1 screenshots
Drawer 1 screenshots
|
||||||||||||||
"type": "patch", | ||||||||||||||
"comment": "Heatmap text color based on contrast ratio", | ||||||||||||||
"packageName": "@fluentui/react-charting", | ||||||||||||||
"email": "[email protected]", | ||||||||||||||
"dependentChangeType": "patch" | ||||||||||||||
} |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { create as d3Create, select as d3Select, Selection } from 'd3-selection'; | ||
import { resolveCSSVariables } from '../../utilities/utilities'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont use relative imports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this pattern at multiple places. Lets start fixing from here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay. But is it correct to use absolute paths? |
||
|
||
/** | ||
* {@docCategory DeclarativeChart} | ||
|
@@ -196,15 +197,6 @@ function cloneLegendsToSVG(chartContainer: HTMLElement, svgWidth: number, svgHei | |
}; | ||
} | ||
|
||
const cssVarRegExp = /var\((--[a-zA-Z0-9\-]+)\)/g; | ||
|
||
function resolveCSSVariables(chartContainer: HTMLElement, styleRules: string) { | ||
const containerStyles = getComputedStyle(chartContainer); | ||
return styleRules.replace(cssVarRegExp, (match, group1) => { | ||
return containerStyles.getPropertyValue(group1); | ||
}); | ||
} | ||
|
||
function svgToPng(svgDataUrl: string, opts: IImageExportOptions = {}): Promise<string> { | ||
return new Promise((resolve, reject) => { | ||
const scale = opts.scale || 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.
🕵🏾♀️ visual regressions to review in the fluentuiv8 Visual Regression Report
Keytip 1 screenshots
react-charting-AreaChart 3 screenshots
react-charting-DonutChart 3 screenshots
react-charting-HeatMapChart 3 screenshots
react-charting-HorizontalBarChart 6 screenshots
react-charting-PieChart 3 screenshots
react-charting-VerticalBarChart 6 screenshots
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 did so many screenshots change
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.
We are using different version of d3-color now (as per yarn.lock file). I can see that reason only.
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.
no we are not. it is mapping to same 3.1.0.
Looks like something else has broken.
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.
Seems unrelated to my changes. let me take a master pull.