rusackas commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3530948772
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -196,61 +185,80 @@ class SimpleEncoder {
}
}
-class WordCloud extends PureComponent<FullWordCloudProps, WordCloudState> {
- static defaultProps = defaultProps;
-
- isComponentMounted = false;
-
- createEncoder = (encoding?: Partial<WordCloudEncoding>): SimpleEncoder =>
- new SimpleEncoder(encoding ?? {}, {
- color: this.props.theme.colorTextLabel,
- fontFamily: this.props.theme.fontFamily,
- fontSize: 20,
- fontWeight: 'bold',
- text: '',
- });
-
- constructor(props: FullWordCloudProps) {
- super(props);
- this.state = {
- words: [],
- scaleFactor: 1,
- };
- this.setWords = this.setWords.bind(this);
- }
-
- componentDidMount() {
- this.isComponentMounted = true;
- this.update();
- }
-
- componentDidUpdate(prevProps: WordCloudProps) {
- const { data, encoding, width, height, rotation } = this.props;
- if (
- !isEqual(prevProps.data, data) ||
- !isEqual(prevProps.encoding, encoding) ||
- prevProps.width !== width ||
- prevProps.height !== height ||
- prevProps.rotation !== rotation
- ) {
- this.update();
- }
- }
-
- componentWillUnmount() {
- this.isComponentMounted = false;
- }
-
- setWords(words: Word[]) {
- if (this.isComponentMounted) {
- this.setState({ words });
+function WordCloud({
+ data,
+ encoding = {},
+ width,
+ height,
+ rotation = 'flat',
+ sliceId,
+ colorScheme,
+ theme,
+}: FullWordCloudProps) {
+ const [words, setWords] = useState<Word[]>([]);
+ const [scaleFactor] = useState(1);
+ const isMountedRef = useRef(true);
+
+ // Store previous props for comparison
+ const prevPropsRef = useRef<{
+ data: PlainObject[];
+ encoding: Partial<WordCloudEncoding>;
+ width: number;
+ height: number;
+ rotation: RotationType;
+ } | null>(null);
+
+ const createEncoder = useCallback(
+ (enc?: Partial<WordCloudEncoding>): SimpleEncoder =>
+ new SimpleEncoder(enc ?? {}, {
+ color: theme.colorTextLabel,
+ fontFamily: theme.fontFamily,
+ fontSize: 20,
+ fontWeight: 'bold',
+ text: '',
+ }),
+ [theme.colorTextLabel, theme.fontFamily],
+ );
+
+ const setWordsIfMounted = useCallback((newWords: Word[]) => {
+ if (isMountedRef.current) {
+ setWords(newWords);
}
- }
-
- update() {
- const { data, encoding } = this.props;
+ }, []);
+
+ const generateCloud = useCallback(
+ (
+ encoder: SimpleEncoder,
+ currentScaleFactor: number,
+ isValid: (word: Word[]) => boolean,
+ ) => {
+ cloudLayout()
+ .size([width * currentScaleFactor, height * currentScaleFactor])
+ .words(data.map((d: Word) => ({ ...d })))
+ .padding(5)
+ .rotate(ROTATION[rotation] || ROTATION.flat)
+ .text((d: PlainObject) => encoder.getText(d))
+ .font((d: PlainObject) => encoder.getFontFamily(d))
+ .fontWeight((d: PlainObject) => encoder.getFontWeight(d))
+ .fontSize((d: PlainObject) => encoder.getFontSize(d))
+ .on('end', (cloudWords: Word[]) => {
+ if (isValid(cloudWords) || currentScaleFactor > MAX_SCALE_FACTOR) {
+ setWordsIfMounted(cloudWords);
+ } else {
+ generateCloud(
+ encoder,
+ currentScaleFactor + SCALE_FACTOR_STEP,
+ isValid,
+ );
+ }
+ })
+ .start();
Review Comment:
The class component had the same uncancelled `end` callback, so this keeps
behavior as-is... this PR is just the hooks conversion, and layout cancellation
is a bigger change than I want to fold in here.
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -262,73 +270,71 @@ class WordCloud extends PureComponent<FullWordCloudProps,
WordCloudState> {
);
const topResults = sortedData.slice(0, topResultsCount);
- this.generateCloud(encoder, 1, (words: Word[]) =>
+ generateCloud(encoder, 1, (cloudWords: Word[]) =>
topResults.every((d: PlainObject) =>
- words.find(({ text }) => encoder.getText(d) === text),
+ cloudWords.find(({ text }) => encoder.getText(d) === text),
),
);
- }
+ }, [data, encoding, createEncoder, generateCloud]);
- generateCloud(
- encoder: SimpleEncoder,
- scaleFactor: number,
- isValid: (word: Word[]) => boolean,
- ) {
- const { data, width, height, rotation } = this.props;
-
- cloudLayout()
- .size([width * scaleFactor, height * scaleFactor])
- .words(data.map((d: Word) => ({ ...d })))
- .padding(5)
- .rotate(ROTATION[rotation] || ROTATION.flat)
- .text((d: PlainObject) => encoder.getText(d))
- .font((d: PlainObject) => encoder.getFontFamily(d))
- .fontWeight((d: PlainObject) => encoder.getFontWeight(d))
- .fontSize((d: PlainObject) => encoder.getFontSize(d))
- .on('end', (words: Word[]) => {
- if (isValid(words) || scaleFactor > MAX_SCALE_FACTOR) {
- this.setWords(words);
- } else {
- this.generateCloud(encoder, scaleFactor + SCALE_FACTOR_STEP,
isValid);
- }
- })
- .start();
- }
+ // Component mount/unmount tracking
+ useEffect(() => {
+ isMountedRef.current = true;
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
- render() {
- const { scaleFactor, words } = this.state;
- const { width, height, encoding, sliceId, colorScheme } = this.props;
-
- const encoder = this.createEncoder(encoding);
-
- const colorFn = CategoricalColorNamespace.getScale(colorScheme);
- const viewBoxWidth = width * scaleFactor;
- const viewBoxHeight = height * scaleFactor;
-
- return (
- <svg
- width={width}
- height={height}
- viewBox={`-${viewBoxWidth / 2} -${viewBoxHeight / 2} ${viewBoxWidth}
${viewBoxHeight}`}
- >
- <g>
- {words.map(w => (
- <text
- key={w.text}
- fontSize={`${w.size}px`}
- fontWeight={w.weight}
- fontFamily={w.font}
- fill={colorFn(encoder.getColor(w as PlainObject), sliceId)}
- textAnchor="middle"
- transform={`translate(${w.x}, ${w.y}) rotate(${w.rotate})`}
- >
- {w.text}
- </text>
- ))}
- </g>
- </svg>
- );
- }
+ // Initial update on mount and when dependencies change
+ useEffect(() => {
+ const prevProps = prevPropsRef.current;
+ const shouldUpdate =
+ !prevProps ||
+ !isEqual(prevProps.data, data) ||
+ !isEqual(prevProps.encoding, encoding) ||
+ prevProps.width !== width ||
+ prevProps.height !== height ||
+ prevProps.rotation !== rotation;
+
Review Comment:
The gate mirrors the class `componentDidUpdate`, which compared these same
five props, so keeping that behavior for the conversion rather than widening it
here.
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -196,61 +185,80 @@ class SimpleEncoder {
}
}
-class WordCloud extends PureComponent<FullWordCloudProps, WordCloudState> {
- static defaultProps = defaultProps;
-
- isComponentMounted = false;
-
- createEncoder = (encoding?: Partial<WordCloudEncoding>): SimpleEncoder =>
- new SimpleEncoder(encoding ?? {}, {
- color: this.props.theme.colorTextLabel,
- fontFamily: this.props.theme.fontFamily,
- fontSize: 20,
- fontWeight: 'bold',
- text: '',
- });
-
- constructor(props: FullWordCloudProps) {
- super(props);
- this.state = {
- words: [],
- scaleFactor: 1,
- };
- this.setWords = this.setWords.bind(this);
- }
-
- componentDidMount() {
- this.isComponentMounted = true;
- this.update();
- }
-
- componentDidUpdate(prevProps: WordCloudProps) {
- const { data, encoding, width, height, rotation } = this.props;
- if (
- !isEqual(prevProps.data, data) ||
- !isEqual(prevProps.encoding, encoding) ||
- prevProps.width !== width ||
- prevProps.height !== height ||
- prevProps.rotation !== rotation
- ) {
- this.update();
- }
- }
-
- componentWillUnmount() {
- this.isComponentMounted = false;
- }
-
- setWords(words: Word[]) {
- if (this.isComponentMounted) {
- this.setState({ words });
+function WordCloud({
+ data,
+ encoding = {},
+ width,
+ height,
+ rotation = 'flat',
+ sliceId,
+ colorScheme,
+ theme,
+}: FullWordCloudProps) {
+ const [words, setWords] = useState<Word[]>([]);
+ const [scaleFactor] = useState(1);
Review Comment:
Restored the scale factor persistence that an earlier refactor dropped, so
the viewBox scales with the layout.
--
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]
