100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918842734
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
If leave animation is disabled, it will still call `parent.remove(el)`
directly, which uses `splice` internally and brings a weird behavior:
existing children: `[elA, elB, elC]`
new children: `[elM, null, elN]`
result: `[merge(elA, elM), elC, elN]`
I've changed my opinion that
> Basically I agree this change, which might make it more intuitive
When setOption, the default strategy is "merge", which is the same as the
other series like line, scatter, ...
The merge strategy in custom series contains:
+ dataItems are merged by index
+ group children are merge by index
In series link line, scatter, graphics are the same between different data
items, and it makes "merge" strategy make sense in most cases. But is custom
series, graphics may not be the same between different data items, which
probably make the "merge" not reasonable.
This change brings a breaking change but only change the `null` case in
group children but not considered the case like "merge graphic unexpectedly of
different data item", "remain properties unexpectedly when merge graphic". I
think it is not good enough.
Could we provide a option on custom series like
```js
series: {
type: 'custom',
merge: false,
}
```
When `merge: false`, there is no merge either between different data items
(see
https://github.com/apache/echarts/blob/5.3.3/src/chart/custom/CustomView.ts#L222)
or in group children.
And it may fix #17349 thoroughly ?
@Ovilia @pissang what's your opinion?
--
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]
