From 603f5d1dde3d87292b73b8ddf930c4155369c83c Mon Sep 17 00:00:00 2001 From: Sergio Clebal Date: Wed, 27 Oct 2021 17:41:46 +0200 Subject: [PATCH 1/4] Fix PieWidget color assignment when using labels prop --- packages/react-ui/src/widgets/PieWidgetUI.js | 33 ++++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/packages/react-ui/src/widgets/PieWidgetUI.js b/packages/react-ui/src/widgets/PieWidgetUI.js index 0c33fbe56..94862819a 100644 --- a/packages/react-ui/src/widgets/PieWidgetUI.js +++ b/packages/react-ui/src/widgets/PieWidgetUI.js @@ -81,21 +81,24 @@ function __generateSerie({ name, data, theme, animation, selectedCategories, lab name, animation, data: data.map((item) => { - if (labels?.[item.name]) { - item.name = labels[item.name]; + // Avoid modify data item + const itemCp = { ...item }; + + if (labels?.[itemCp.name]) { + itemCp.name = labels[itemCp.name]; } const disabled = - selectedCategories?.length && !selectedCategories.includes(item.name); + selectedCategories?.length && !selectedCategories.includes(itemCp.name); if (disabled) { - disableSerie(item, theme); - return item; + disableSerie(itemCp, theme); + return itemCp; } - setColor(item); + setColor(itemCp); - return item; + return itemCp; }), radius: ['74%', '90%'], selectedOffset: 0, @@ -175,19 +178,23 @@ function PieWidgetUI({ const dataWithColor = useMemo(() => { return (data || []).map((item) => { + const colorByCategoryCp = { ...colorByCategory }; const { name } = item; - const colorUsed = colorByCategory[name]; + const colorUsed = colorByCategoryCp[name]; if (colorUsed) { item.color = colorUsed; } else { - const colorsToUse = colors || theme.palette.qualitative.bold; - colorByCategory[name] = - colorsToUse[Object.keys(colorByCategory).length] || '#fff'; - setColorByCategory({ ...colorByCategory }); + const paletteToUse = colors || theme.palette.qualitative.bold; + const colorToUse = paletteToUse[Object.keys(colorByCategoryCp).length] || '#fff'; + colorByCategoryCp[name] = colorToUse; + item.color = colorToUse; + setColorByCategory(colorByCategoryCp); } return item; }); - }, [data, colorByCategory, colors, theme.palette.qualitative.bold]); + // Use colorByCategory as dependency cause unnecesary useEffect repetition + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [data, colors, theme.palette.qualitative.bold]); useEffect(() => { const config = __generateDefaultConfig({ formatter, tooltipFormatter }, theme); From ec24a86d4bb1dc4f23ab35a8ff041624c6100e48 Mon Sep 17 00:00:00 2001 From: Sergio Clebal Date: Wed, 27 Oct 2021 17:43:17 +0200 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7c9d8727..b6032cfbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Improve TS typings [#213](https://github.com/CartoDB/carto-react/pull/213) - Fix first X axis value partially hidden in Histogram widget [#215](https://github.com/CartoDB/carto-react/pull/215) - Fix animation duration not consistent in TimeSeriesWidget [#214](https://github.com/CartoDB/carto-react/pull/214) +- Fix PieWidget color assignment when using labels prop [#218](https://github.com/CartoDB/carto-react/pull/218) ## 1.1.0-beta.2 (2021-10-22) From a6f5624df2b67c996203871eca395c5cf693c6c1 Mon Sep 17 00:00:00 2001 From: Sergio Clebal Date: Wed, 27 Oct 2021 17:46:57 +0200 Subject: [PATCH 3/4] Code improvements --- packages/react-ui/src/widgets/PieWidgetUI.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/react-ui/src/widgets/PieWidgetUI.js b/packages/react-ui/src/widgets/PieWidgetUI.js index 94862819a..91a127a3e 100644 --- a/packages/react-ui/src/widgets/PieWidgetUI.js +++ b/packages/react-ui/src/widgets/PieWidgetUI.js @@ -158,6 +158,7 @@ function PieWidgetUI({ }); const [elementHover, setElementHover] = useState(); let defaultLabel = useRef({}); + const colorByCategory = useRef({}); const updateLabel = (params) => { const echart = chartInstance.current.getEchartsInstance(); @@ -169,31 +170,26 @@ function PieWidgetUI({ echart.setOption(option, true); }; - const [colorByCategory, setColorByCategory] = useState({}); - - // Reset color by category when colors changes + // Reset colorByCategory when colors changes // Spread colors array to avoid reference problems // eslint-disable-next-line react-hooks/exhaustive-deps - useEffect(() => setColorByCategory({}), [...(colors || [])]); + useEffect(() => (colorByCategory.current = {}), [...(colors || [])]); const dataWithColor = useMemo(() => { return (data || []).map((item) => { - const colorByCategoryCp = { ...colorByCategory }; const { name } = item; - const colorUsed = colorByCategoryCp[name]; + const colorUsed = colorByCategory.current[name]; if (colorUsed) { item.color = colorUsed; } else { const paletteToUse = colors || theme.palette.qualitative.bold; - const colorToUse = paletteToUse[Object.keys(colorByCategoryCp).length] || '#fff'; - colorByCategoryCp[name] = colorToUse; + const colorToUse = + paletteToUse[Object.keys(colorByCategory.current).length] || '#fff'; + colorByCategory.current[name] = colorToUse; item.color = colorToUse; - setColorByCategory(colorByCategoryCp); } return item; }); - // Use colorByCategory as dependency cause unnecesary useEffect repetition - // eslint-disable-next-line react-hooks/exhaustive-deps }, [data, colors, theme.palette.qualitative.bold]); useEffect(() => { From 07452d7a3389d4074c5897c3cdebe80c20ac07e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Wed, 27 Oct 2021 19:27:17 +0200 Subject: [PATCH 4/4] Refactor to rename internal variable --- packages/react-ui/src/widgets/PieWidgetUI.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/react-ui/src/widgets/PieWidgetUI.js b/packages/react-ui/src/widgets/PieWidgetUI.js index 91a127a3e..852c4ad08 100644 --- a/packages/react-ui/src/widgets/PieWidgetUI.js +++ b/packages/react-ui/src/widgets/PieWidgetUI.js @@ -82,23 +82,23 @@ function __generateSerie({ name, data, theme, animation, selectedCategories, lab animation, data: data.map((item) => { // Avoid modify data item - const itemCp = { ...item }; + const clonedItem = { ...item }; - if (labels?.[itemCp.name]) { - itemCp.name = labels[itemCp.name]; + if (labels?.[clonedItem.name]) { + clonedItem.name = labels[clonedItem.name]; } const disabled = - selectedCategories?.length && !selectedCategories.includes(itemCp.name); + selectedCategories?.length && !selectedCategories.includes(clonedItem.name); if (disabled) { - disableSerie(itemCp, theme); - return itemCp; + disableSerie(clonedItem, theme); + return clonedItem; } - setColor(itemCp); + setColor(clonedItem); - return itemCp; + return clonedItem; }), radius: ['74%', '90%'], selectedOffset: 0,