Skip to content
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.

Sign up for GitHub

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

Fix PieWidget color assignment when using labels prop #218

Merged
merged 5 commits into from
Oct 27, 2021
Merged

Fix PieWidget color assignment when using labels prop #218

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
603f5d1
Fix PieWidget color assignment when using labels prop
Clebal Oct 27, 2021
ec24a86
Update CHANGELOG.md
Clebal Oct 27, 2021
a6f5624
Code improvements
Clebal Oct 27, 2021
07452d7
Refactor to rename internal variable
VictorVelarde Oct 27, 2021
9dc0d2a
Merge branch 'master' into bug/fix-pie-widget-color-assignment-when-u…
VictorVelarde Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
37 changes: 20 additions & 17 deletions packages/react-ui/src/widgets/PieWidgetUI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 clonedItem = { ...item };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, I've just done a small renamed of the cloned item

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually use suffix Cp to indicate that's a copy. But I see your option also ok.


if (labels?.[clonedItem.name]) {
clonedItem.name = labels[clonedItem.name];
}

const disabled =
selectedCategories?.length && !selectedCategories.includes(item.name);
selectedCategories?.length && !selectedCategories.includes(clonedItem.name);

if (disabled) {
disableSerie(item, theme);
return item;
disableSerie(clonedItem, theme);
return clonedItem;
}

setColor(item);
setColor(clonedItem);

return item;
return clonedItem;
}),
radius: ['74%', '90%'],
selectedOffset: 0,
Expand Down Expand Up @@ -155,6 +158,7 @@ function PieWidgetUI({
});
const [elementHover, setElementHover] = useState();
let defaultLabel = useRef({});
const colorByCategory = useRef({});

const updateLabel = (params) => {
const echart = chartInstance.current.getEchartsInstance();
Expand All @@ -166,28 +170,27 @@ 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 { name } = item;
const colorUsed = colorByCategory[name];
const colorUsed = colorByCategory.current[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(colorByCategory.current).length] || '#fff';
colorByCategory.current[name] = colorToUse;
item.color = colorToUse;
}
return item;
});
}, [data, colorByCategory, colors, theme.palette.qualitative.bold]);
}, [data, colors, theme.palette.qualitative.bold]);

useEffect(() => {
const config = __generateDefaultConfig({ formatter, tooltipFormatter }, theme);
Expand Down