You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2020/08/17 08:41:11 UTC
[GitHub] [incubator-echarts] easonyq opened a new pull request #13139: Fix: make `contentToOption` totally optional
easonyq opened a new pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
<!-- USE ONCE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
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.
<!-- LIST THE API CHANGES HERE -->
No
### Related test cases or examples to use the new APIs
NA.
## Others
### Merging options
- [x] Please squash the commits into a single one when merge.
### Other information
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq edited a comment on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq edited a comment on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674748442
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 #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 #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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq commented on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq commented on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-675216996
Thanks for @pissang 's review. I've applied another patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#discussion_r471422952
##########
File path: src/component/toolbox/feature/DataView.ts
##########
@@ -371,6 +371,13 @@ class DataView extends ToolboxFeature<ToolboxDataViewFeatureOption> {
addEventListener(closeButton, 'click', close);
addEventListener(refreshButton, 'click', function () {
+ if ((typeof contentToOption === 'undefined' && typeof optionToContent !== 'undefined') ||
+ (typeof contentToOption !== 'undefined' && typeof optionToContent === 'undefined')) {
+ console.warn('It seems you have just provided one of `contentToOption` and `optionToContent` functions but missed the other one. Data change is ignored.')
+ close();
Review comment:
Usually warn is only displayed in dev mode.
So a __DEV__ condition should be wrapped here.
```ts
if (__DEV__) {
console.warn('....');
}
```
##########
File path: src/component/toolbox/feature/DataView.ts
##########
@@ -371,6 +371,13 @@ class DataView extends ToolboxFeature<ToolboxDataViewFeatureOption> {
addEventListener(closeButton, 'click', close);
addEventListener(refreshButton, 'click', function () {
+ if ((typeof contentToOption === 'undefined' && typeof optionToContent !== 'undefined') ||
+ (typeof contentToOption !== 'undefined' && typeof optionToContent === 'undefined')) {
Review comment:
It will be better to use `null` check here
```
if (contentToOption == null ...) {}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq edited a comment on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq edited a comment on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674748442
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 **textarea is not appended to the document** in this case. So 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 #13031), it is difficult (or saying impossible) for us to parse it and get the right data we want.
3. By contrast, let's image 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.
3. 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq edited a comment on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq edited a comment on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674748442
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 and destroy the data. (All bugs mentioned in #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 #13031), it is difficult (or saying impossible) for us to parse it and get the right data we want.
3. By contrast, let's image 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.
3. 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang merged pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq commented on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq commented on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674748442
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 point here is:
1. If user has provided `optionToContent` function but missed `contentToOption` function, the system tried to parse the content in **textarea**. But actually **textarea is not appended to the document** in this case. So 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 #13031), it is difficult (or saying impossible) for us to parse it and get the right data we want.
3. By contrast, let's image 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.
3. 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-680542542
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq edited a comment on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq edited a comment on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674748442
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 system tried to parse the content in **textarea**. But actually **textarea is not appended to the document** in this case. So 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 #13031), it is difficult (or saying impossible) for us to parse it and get the right data we want.
3. By contrast, let's image 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.
3. 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] easonyq edited a comment on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
easonyq edited a comment on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674748442
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 #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 #13031), it is difficult (or saying impossible) for us to parse it and get the right data we want.
3. By contrast, let's image 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.
3. 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on pull request #13139: Fix: make `contentToOption` totally optional
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #13139:
URL: https://github.com/apache/incubator-echarts/pull/13139#issuecomment-674833206
Hi @easonyq, thanks for the detailed explanation. I think the scenario you described is complete enough.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org