rusackas commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3534182888
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonChart.tsx:
##########
@@ -106,35 +88,36 @@ class HorizonChart extends
PureComponent<HorizonChartProps> {
const rawExtent = d3Extent(allValues, d => d.y);
// Only set yDomain if we have valid min and max values
if (rawExtent[0] != null && rawExtent[1] != null) {
- yDomain = [rawExtent[0], rawExtent[1]];
+ return [rawExtent[0], rawExtent[1]];
}
}
+ return undefined;
+ }, [colorScale, data]);
- return (
- <StyledDiv>
- <div
- className={`superset-legacy-chart-horizon ${className}`}
- style={{ height }}
- >
- {data.map(row => (
- <HorizonRow
- key={row.key.join(',')}
- width={width}
- height={seriesHeight}
- title={ensureIsArray(row.key).join(', ')}
- data={row.values}
- bands={bands}
- colors={colors}
- colorScale={colorScale}
- mode={mode}
- offsetX={offsetX}
- yDomain={yDomain}
- />
- ))}
- </div>
- </StyledDiv>
- );
- }
+ return (
+ <StyledDiv>
+ <div
+ className={`superset-legacy-chart-horizon ${className}`}
+ style={{ height }}
+ >
+ {data.map(row => (
+ <HorizonRow
+ key={row.key.join(',')}
+ width={width}
+ height={seriesHeight}
+ title={ensureIsArray(row.key).join(', ')}
Review Comment:
That key/join mismatch predates this PR, the class component had the exact
same `row.key.join(',')` next to `ensureIsArray(row.key)`. Out of scope for a
mechanical FC conversion.
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx:
##########
@@ -52,162 +50,140 @@ interface HorizonRowProps {
yDomain?: [number, number];
}
-const defaultProps: Partial<HorizonRowProps> = {
- className: '',
- width: 800,
- height: 20,
- bands: DEFAULT_COLORS.length >> 1,
- colors: DEFAULT_COLORS,
- colorScale: 'series',
- mode: 'offset',
- offsetX: 0,
- title: '',
- yDomain: undefined,
-};
-
-class HorizonRow extends PureComponent<HorizonRowProps> {
- static defaultProps = defaultProps;
-
- private canvas: HTMLCanvasElement | null = null;
-
- componentDidMount() {
- this.drawChart();
- }
-
- componentDidUpdate() {
- this.drawChart();
- }
-
- componentWillUnmount() {
- this.canvas = null;
- }
-
- drawChart() {
- if (this.canvas) {
- const {
- data: rawData,
- yDomain,
- width = 800,
- height = 20,
- bands = DEFAULT_COLORS.length >> 1,
- colors = DEFAULT_COLORS,
- colorScale,
- offsetX = 0,
- mode,
- } = this.props;
-
- const data =
- colorScale === 'change'
- ? rawData.map(d => ({ ...d, y: d.y - rawData[0].y }))
- : rawData;
-
- const context = this.canvas.getContext('2d');
- if (!context) return;
- context.imageSmoothingEnabled = false;
- context.clearRect(0, 0, width, height);
- // Reset transform
- context.setTransform(1, 0, 0, 1, 0, 0);
- context.translate(0.5, 0.5);
-
- const step = width / data.length;
- // the data frame currently being shown:
- const startIndex = Math.floor(Math.max(0, -(offsetX / step)));
- const endIndex = Math.floor(
- Math.min(data.length, startIndex + width / step),
- );
-
- // skip drawing if there's no data to be drawn
- if (startIndex > data.length) {
- return;
+function HorizonRow({
+ className = '',
+ width = 800,
+ height = 20,
+ data: rawData,
+ bands = DEFAULT_COLORS.length >> 1,
+ colors = DEFAULT_COLORS,
+ colorScale = 'series',
+ mode = 'offset',
+ offsetX = 0,
+ title = '',
+ yDomain,
+}: HorizonRowProps) {
+ const canvasRef = useRef<HTMLCanvasElement | null>(null);
+
+ const drawChart = useCallback(() => {
+ const canvas = canvasRef.current;
+ if (!canvas) return;
+
+ const data =
+ colorScale === 'change' && rawData.length > 0
+ ? rawData.map(d => ({ ...d, y: d.y - rawData[0].y }))
+ : rawData;
+
+ const context = canvas.getContext('2d');
+ if (!context) return;
+ context.imageSmoothingEnabled = false;
+ context.clearRect(0, 0, width, height);
+ // Reset transform
+ context.setTransform(1, 0, 0, 1, 0, 0);
+ context.translate(0.5, 0.5);
+
+ const step = width / data.length;
+ // the data frame currently being shown:
+ const startIndex = Math.floor(Math.max(0, -(offsetX / step)));
+ const endIndex = Math.floor(
+ Math.min(data.length, startIndex + width / step),
+ );
+
+ // skip drawing if there's no data to be drawn
+ if (startIndex > data.length) {
+ return;
+ }
+
+ // Create y-scale
+ const [min, max] =
+ yDomain || (d3Extent(data, d => d.y) as [number, number]);
+ const y = scaleLinear()
+ .domain([0, Math.max(-min, max)])
+ .range([0, height]);
Review Comment:
Same story, the step/extent math is byte-for-byte what the class component
did. Not something this refactor introduced, out of scope here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
