Fix hiding chart elements when components are removed #2529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 90888aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tsck
changed the title
tsck
changed the title
tsck
commented
Oct 30, 2024
Comment on lines
+25
to
+27
| updatedOptions.series = [ | ||
| ...updatedOptions.series.filter(series => series.name !== name), | ||
| ]; |
There was a problem hiding this comment.
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.
bruugey
reviewed
Oct 30, 2024
| @@ -26,6 +26,24 @@ export function Grid({ horizontal = true, vertical = true }: GridProps) { | |||
| updatedOptions.xAxis = getUpdatedLineOptions(!!vertical); | |||
| updatedOptions.yAxis = getUpdatedLineOptions(!!horizontal); | |||
| updateChartOptions(updatedOptions); | |||
|
|
|||
| return () => { | |||
There was a problem hiding this comment.
This does not get unmounted by default?
There was a problem hiding this comment.
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.
bruugey
reviewed
Oct 30, 2024
|
|
||
| return () => { | ||
| /** | ||
| * Hides the axis when the component is unmounted. |
There was a problem hiding this comment.
Is there a cleaner way to handle unmount?
There was a problem hiding this comment.
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!
|
Size Change: +264 B (+0.02%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
| * Remove the series when the component unmounts to make sure the series | ||
| * is removed when a `Line` is hidden. | ||
| */ | ||
| removeChartSeries(name); |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Probably a check within addChartSeries that logs something to the console, and exits early?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
tsck
requested review from
bruugey and
TheSonOfThomp
and removed request for
stephl3
| @@ -14,6 +14,17 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => { | |||
| expect(updatedOptions.series?.[1].name).toBe(newSeriesName); | |||
| }); | |||
|
|
|||
| test('should not add a series if a chart with the same name exists', () => { | |||
There was a problem hiding this comment.
nit: add the name of the util we're testing to the test name (i.e. "addSeries should ..."
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
Example
Code
On Render
After 5 seconds
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changesFor new components
🧪 How to test changes