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.

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

[5.0] [FEATURE] Remove component #12987

Merged
merged 17 commits into from Jul 18, 2020
Merged

[5.0] [FEATURE] Remove component #12987

merged 17 commits into from Jul 18, 2020

Conversation

100pah
Copy link
Member

@100pah 100pah commented Jul 17, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Support replaceMerge in setOption

Provide a new mode in setOption to support removal of some components.

After this PR, there are three mode about component update:

  1. normalMerge (already has)
chart.setOption(option);

Most common used. The existing component will NEVER be removed.
The components declared in the input option will be merged to some existing component by id/name/index, or create new component if not able to be merged.

  1. notMerge (already has)
chart.setOption(option, true);
// or
chart.setOption(option, { notMerge: true });

Replace all of the existing models with the new option, no state in models are kept.

  1. replaceMerge (new)
chart.setOption(option, {
    // specify component "mainType" here.
    replaceMerge: 'series'
});
chart.setOption(option, {
    // specify components "mainType" here.
    replaceMerge: ['xAxis', 'yAxis', 'grid']
});

The specified component (by mainType) will be performed replaceMerge, others will go with normalMerge.
replaceMerge means:
Within the scope of a certain mainType, the existing components will be merged with the new component declarations (in the input option) only if they have id matched. All of the other existing components will be removed, and all of the other new components will be created on the empty componentIndex.

By the mechanism of replaceMerge, we can remove a certain mainType of components, or totally replace them with new components, or partially replace and partially remain.

Support option timeline.replaceMerge

See show case below for details.

Others fix and enhancement

  • DataZoom enhancement:
    • Including both "slider dataZoom", "inside dataZoom", "toolbox dataZoom".
    • Make it works when some of the axis removed, or some of the dataZoom removed.
    • Supports referencing axes by axis id.
    • DataZoom supports referencing all axes by like xAxisIndex: 'all', referencing none axis by like xAxisIndex: 'none' or xAxisIndex: false.
    • DataZoom "auto axis selection" logic previously only work on the first setOption. Now make it works on multiple time of setOption.
    • Some refactor to simplify the code.
  • Refactor some of the component reference logic to make their logic the same in detail.
  • Try to support the reproduce an chart instance in other chart instance in some simple cases (via getOption). The state of the chart instance (like zoom state) will be reproduced.

Show case

Previously, the interaction state can not be kept if using notMerge mode:
facet

At present, the interaction state can be kept when using replaceMerge mode:
facet-new

At present, we are able to add and remove coordinate systems freely, and make dataZoom controls axis as expected. That might be some basic of storytelling demo. Previously, there were several bugs not also about coord sys but also about toolbox and dataZoom:
transition

At present, series can be removed, while other not related series and components keeping their state:

dynamic-series

Previously, there is a timeline bug when different time tick has different number of series (series can be added but can not be reduced):
timeline-dy

At present, series can be removed in timeline:
timeline-dy-new

Related test cases

<test/option-replaceMerge2.html>
<test/option-replaceMerge.html>
<test/dataZoom-feature.html>

Memento

Whether to change the component index after other components removed?

The questions is: whether the componentIndex of should be changed after some component removed via setOption in replaceMode.

There has been some discussion about it, but not clear at that time because not touch the code enough then.

After trying to implement it, found something more clearly:
Suppose there has been component: [A, B, C, D].
That means: A.componentIndex === 0, B.componentIndex === 1, ...
Then call setOption in replaceMerge mode to remove B, C and add X.

Strategy of change the index

The final component indices may be like: [A, X, D], or it also may be [A, D, X].
There might be no easy and intuitive way to make users aware what the final indices are.

Strategy of keep the index

The final component indices may be like: [A, X, -, D], or it also may be [A, -, -, D, X].
Users are also not easy to know what D.componentIndex is. But at lease, users can know that the existing A and D do not change their componentIndex. Thus the "index reference" (like xAxisIndex, yAxisIndex) in option about A and D will not be broken.

So we can probably found that:
(1) In either of the two strategy, users are probably not able to know clearly what the final indices like if the second setOption both added and removed something. Therefore, if users meet this kind of scenario, it's not a good idea to use index to reference components. On the contrary, using id should be recommended.
(2) [strategy_keep_index] do not break the previous "index reference".
(3) [strategy_change_index] might make the code more brittle, because it forbids any of the persistence (like hash map key) are relevant to componentIndex, seriesIndex (because it might be changed). It not easy to always ensure that and might not easy to be found in test stage.

So finally, chose [strategy_keep_index]. And in this strategy, the "hole" might exists. New components are first to fill the hole.

TODO: Do we need notMerge: ['xxx']

notMerge: ['series'] seems more intuitive than replaceMerge: ['series'] when just need to totally replace series.
And if we only want to reset a type of components,
if we use replaceMerge: ['dataZoom'], we need to make sure id is not specified or change a different id.
Comparatively notMerge: ['dataZoom'] is easy a bit.

TODO: about the "Restore" feature

"Restore" feature means the toolbox.feature.restore and the action "restore".
Previously the implementation of "restore" is not a complete solution, because after the second setOption, in some case it can not restore to exactly the original state.
The reason is, the original requirement expect that "only restore the user interaction" but "do not restore the setOption result" (setOption is always used to update the data).
If these things below happen one by one:

orgianl_state => user_zoom_data_1 => call_setOption_updated_data_1 => user_zoom_data_2 => call_setOption_updated_data_2

After "restore" click, we expect the final state exactly the same as:
orgianl_state => call_setOption_updated_data_1 => call_setOption_updated_data_2

The previous implementation is temporary and not totally correct and not easy to be enhanced to support the new feature "replaceMerge".
So we do not support it at present and remove the support of "restore" after the second setOption temporarily.
That is a TODO .

Other information

Related issues: #6202 #4356 #4033 #8246 #3002

related discussion

100pah added 17 commits July 7, 2020 13:51
Use `ecMode.getComponent` `ecModel.queryComponents` instead.
Because:
(1) `dependentModes` is rarely used.
(2) It might be wrongly cached. If cached, it might not be updated when new `setOption`.
(3) Not necessary to prepare all dependencies model table for each component. Should better to require them when really want to use them.
(1) support axis id reference in dataZoom. (except toolbox dataZoom)
(2) enhance dataZoom auto choose axis logic to support multiple setOption. (except toolbox dataZoom)
(3) enhance dataZoom to enable axis and dataZoom removal. (except toolbox dataZoom)
(4) simplify the code of dataZoom.
(1) support toolbox dataZoom works on the second setOption.
(2) change the mechanism of "internal component" to support adding them dynamically.
(3) uniform the "get referring component".
(4) support toolbox dataZoom use axis id to refer axis (previously only axisIndex can be used).
(5) remove the support to restore on the second setOption temporarily.
# Conflicts:
#	src/model/Global.ts
@echarts-bot
Copy link

echarts-bot bot commented Jul 17, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

@100pah 100pah merged commit 82015b7 into next Jul 18, 2020
@echarts-bot
Copy link

echarts-bot bot commented Jul 18, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants