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

Jump to bottom

Fix hiding chart elements when components are removed #2529

Merged
merged 12 commits into from
Nov 1, 2024
Merged

Fix hiding chart elements when components are removed #2529

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
473b558
Add fix
tsck Oct 30, 2024
9d82b66
Cleanup Grid
tsck Oct 30, 2024
9611398
Fix axes and grid
tsck Oct 30, 2024
3a3f230
Changeset
tsck Oct 30, 2024
d6bc99e
Move unset options into var
tsck Oct 30, 2024
06ba916
Prevent series duplication
tsck Oct 30, 2024
8c2dc53
Add test
tsck Oct 30, 2024
f05f364
Merge remote-tracking branch 'origin' into fix-updates
tsck Oct 30, 2024
e08d2f2
Update patch description
tsck Oct 30, 2024
436c01f
Fix test description
tsck Oct 30, 2024
d78a0d5
Merge remote-tracking branch 'origin' into fix-updates
tsck Oct 31, 2024
90888aa
More descriptive test names
tsck Nov 1, 2024
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
5 changes: 5 additions & 0 deletions .changeset/nice-radios-move.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@lg-charts/core': patch
---

Fixes hiding chart elements when components are removed.
6 changes: 3 additions & 3 deletions charts/core/src/Chart/hooks/updateUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ export function removeSeries(
const updatedOptions = { ...currentOptions };

if (updatedOptions.series) {
updatedOptions.series = updatedOptions.series.filter(
series => series.name !== name,
);
updatedOptions.series = [
...updatedOptions.series.filter(series => series.name !== name),
];
Comment on lines +27 to +29
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was added just to be on the safe side. I don't believe echarts actually does an object reference equality check to control repaints, but thought it might as well be a new object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

}

return updatedOptions;
Expand Down
7 changes: 6 additions & 1 deletion charts/core/src/Chart/hooks/useChart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,12 @@ export function useChart({ theme, onChartReady }: ChartHookProps) {
const updateChartRef = useMemo(
() =>
debounce((chartOptions: Partial<ChartOptions>) => {
chartInstanceRef.current?.setOption(chartOptions);
/**
* The second argument is `true` to merge the new options with the existing ones.
* This is needed to ensure that series get removed properly.
* See issue: https://github.com/apache/echarts/issues/6202
* */
chartInstanceRef.current?.setOption(chartOptions, true);
}, 50),
[],
);
Expand Down
18 changes: 18 additions & 0 deletions charts/core/src/Grid/Grid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,24 @@ export function Grid({ horizontal = true, vertical = true }: GridProps) {
updatedOptions.xAxis = getUpdatedLineOptions(!!vertical);
updatedOptions.yAxis = getUpdatedLineOptions(!!horizontal);
updateChartOptions(updatedOptions);

return () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not get unmounted by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This issue is that this isn't a real component, in a sense, given that it doesn't actually render anything to the DOM. Instead, this ends up basically being "config via JSX". So on unmount, essentially nothing happens. The component itself, which renders nothing, IS removed from the DOM. But this does nothing to actually update the chart that's rendered, since the chart is config based.

/**
* Hides the grid lines when the component is unmounted.
*/
updateChartOptions({
xAxis: {
splitLine: {
show: false,
},
},
yAxis: {
splitLine: {
show: false,
},
},
});
};
}, [horizontal, vertical, theme]);

return null;
Expand Down
10 changes: 9 additions & 1 deletion charts/core/src/Line/Line.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,22 @@ import { defaultLineOptions } from './config';
import { LineProps } from './Line.types';

export function Line({ name, data }: LineProps) {
const { addChartSeries } = useChartContext();
const { addChartSeries, removeChartSeries } = useChartContext();

useEffect(() => {
addChartSeries({
...defaultLineOptions,
name,
data,
});

return () => {
/**
* Remove the series when the component unmounts to make sure the series
* is removed when a `Line` is hidden.
*/
removeChartSeries(name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only place I see a potential issue is if the consumer has rendered another line with the same name before this one unmounts, and we then remove the wrong line from the chart. But at that point, I'd almost call that "user error"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is an interesting point. I've discovered that when lines are rendered with the same names, they end up having the same colors. I wonder if there's actually any good way to restrict this so that names have to be unique

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably a check within addChartSeries that logs something to the console, and exits early?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah and actually addChartSeries is buggy atm. It adds duplicates on re-renders. Was going to fix that in a followup but maybe I should just fix that here. Will push up this fix shortly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So oddly I'm finding that addChartSeries ends up getting called twice on render, even if the useEffect in Line only runs once. I'm kind of confused as to why 🤔. That said, this means that even legit series are sometimes getting added twice. I've added logic to prevent duplication and updated the docs to say that it needs to be unique and that non-uniquely named series won't be rendered, however I didn't add an error log due to it triggering on legit cases. Do you think that's enough?

};
}, [name, data]);

return null;
Expand Down
16 changes: 16 additions & 0 deletions charts/core/src/XAxis/XAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,22 @@ export function XAxis({ type, label, unit }: XAxisProps) {

useEffect(() => {
updateChartOptions(getOptions({ type, label, unit, theme }));

return () => {
/**
* Hides the axis when the component is unmounted.
*/
updateChartOptions({
xAxis: {
axisLine: {
show: false,
},
axisLabel: {
show: false,
},
},
});
};
}, [type, label, unit, theme, updateChartOptions]);

return null;
Expand Down
16 changes: 16 additions & 0 deletions charts/core/src/YAxis/YAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,22 @@ export function YAxis({ type, label, unit }: YAxisProps) {

useEffect(() => {
updateChartOptions(getOptions({ type, label, unit, theme }));

return () => {
/**
* Hides the axis when the component is unmounted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a cleaner way to handle unmount?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. Definitely open to ideas! The problem is as noted in this comment. So the goal here is to essentially manually trigger a chart config update any time the component unmounts.

I'll actually chuck this out to the team async and see if anyone has any cleaner ideas for this!

*/
updateChartOptions({
yAxis: {
axisLine: {
show: false,
},
axisLabel: {
show: false,
},
},
});
};
}, [type, label, unit, theme, updateChartOptions]);

return null;
Expand Down
Loading