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

Fix: make contentToOption totally optional #13139

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Conversation

easonyq
Copy link

@easonyq easonyq commented Aug 17, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fix #13031 and #13086

Fixed issues

#13031 has described such a situation that user has provided optionToContent function while has not provided the corresponding contentToOption function.
If we think echarts should also work well in this situation, we need to make contentToOption function totally optional. (In other words, if user hasn't provided it, bugs like missing series data or showing a list of undefined should not happen.)

Details

Before: What was the problem?

If user provide optionToContent function but has not provided contentToOption function, press 'refresh' will lose data and make a list of undefined displayed.

After: How is it fixed in this PR?

Even user has not provided contentToOption function, by pressing 'refresh' button also works well.

Usage

Are there any API changes?

  • The API has been changed.

No

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@easonyq
Copy link
Author

easonyq commented Aug 17, 2020

In #13086 @pissang has raised an issue. He says that PR will make option not refreshed if user didn't provide optionToContent or contentToOption function and changed the default text area. This PR also fix that bug.

The key points here are:

  1. If user has provided optionToContent function but missed contentToOption function, the original system will try to parse the content in textarea. But actually in this case textarea is not appended to the document at all. So textarea.value is an empty string, parsing and applying this will destroy the data. (All bugs mentioned in toolbox optionToContent 自定义 dataView,点刷新,optionToContent回调中拿到的series数据遗失 #13031 such as one line missing or a list of undefined are all casued by this reason).
    The content we need to parse is a user-provided HTML what optionToContent returned, rather than the default textarea.
  2. Futhermore, if user provided a complicated HTML structure (such as a table described in toolbox optionToContent 自定义 dataView,点刷新,optionToContent回调中拿到的series数据遗失 #13031), it is difficult (or saying impossible) for us to parse it and get the right data we want.
  3. By contrast, let's imagine the opposite situation that user has provided contentToOption function but missed optionToContent function. In this case, the default textarea will show but the parsing function is overwritten by user, which is totally unnecessary.
  4. Given these facts, in my opinion user should still use optionToContent along with contentToOption together. Thus this PR gives warning when user missed one of these two. If he did, a warning message will be shown and all data will remain the same.
  5. Of course user can choose to provide none of these two. This means he are about to use default textarea and default parsing function. This still works.

src/component/toolbox/feature/DataView.ts Outdated Show resolved Hide resolved
src/component/toolbox/feature/DataView.ts Show resolved Hide resolved
@pissang
Copy link
Contributor

pissang commented Aug 17, 2020

Hi @easonyq, thanks for the detailed explanation. I think the scenario you described is complete enough.

@easonyq
Copy link
Author

easonyq commented Aug 18, 2020

Thanks for @pissang 's review. I've applied another patch.

@pissang pissang merged commit 6af8923 into apache:next Aug 26, 2020
@echarts-bot
Copy link

echarts-bot bot commented Aug 26, 2020

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

@easonyq easonyq deleted the fix-13031 branch November 24, 2020 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants