-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
Support using dataType param to specify edge highlighting #16243
Conversation
Thanks for your contribution! Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the |
Please dont commit yarn.lock |
Please don't commit dist and i18n folder. Just the changed source code |
@Dingzhaocheng I didn't mean deleting these two folders. Just don't commit the change. Make sure in the |
470a03b
to
7282288
Compare
I'm sorry, I didn't get it right earlier. 😊 |
Never mind. We are glad to having your contribution |
src/view/Chart.ts
Outdated
@@ -152,7 +152,7 @@ class ChartView { | |||
* Highlight series or specified data item. | |||
*/ | |||
highlight(seriesModel: SeriesModel, ecModel: GlobalModel, api: ExtensionAPI, payload: Payload): void { | |||
toggleHighlight(seriesModel.getData(), payload, 'emphasis'); | |||
toggleHighlight(seriesModel.getData(payload && payload.dataType), payload, 'emphasis'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Developers may give wrong dataType that is not supported in the echarts. It's better to check if data
exists here.
Also downplay
method should also add this change.
a782e2f
to
b929a18
Compare
src/view/Chart.ts
Outdated
@@ -152,14 +152,14 @@ class ChartView { | |||
* Highlight series or specified data item. | |||
*/ | |||
highlight(seriesModel: SeriesModel, ecModel: GlobalModel, api: ExtensionAPI, payload: Payload): void { | |||
toggleHighlight(seriesModel.getData(), payload, 'emphasis'); | |||
toggleHighlight(seriesModel.getData(payload && payload.dataType), payload, 'emphasis'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to do null checking. We should always do it at the place where it is used. In this case is before we passing it as a parameter to toggleHighlight
. What if seriesModel.getData()
has other methods to fetch the data if you do null checking in the getLinkedData
. ?
Here is an example:
const data = seriesModel.getData(payload && payload.dataType)
if (!data) {
if (__DEV__) {
error(`Unkown dataType ${dataType}`);
}
return;
}
toggleHighlight(data, payload, 'emphasis');
Null checking in the getLinkedData
can be removed to simplify the code.
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
@Dingzhaocheng hi, how can we highlight both nodes and edges simultaneously? |
@pissang please help |
You can see the PR changes file, you can follow these changes to directly modify your local package, then you can use the new API on the proposal, the test case also has the usage, you can refer to it |
I'm not sure if the new version already has this method, you can check the documentation, if so, then just update to the new version |
@Dingzhaocheng i am using the latest version. The "dataType" property is working but I want to highlight both nodes and edges at the same time. Currently it is allowing me to highlight only one of them at a time. Can you suggest something? |
Modify the local source code, change the dataType in the payload, pass in an array structure to indicate that the nodes and edges are highlighted together, and execute toggleHighlight in a loop based on the payload in the highlight method; not elegant, but effective |
Brief Information
This pull request is in the type of:
What does this PR do?
Added property configuration to specify edge highlighting
Fixed issues
Details
Before: What was the problem?
Source from this issue: #16134 (comment), need to solve the problem of highlighting on the specified side
After: How is it fixed in this PR?
According to the original method "highlight" of the highlighted node, pass in the dataType property in the payload
Misc
Related test cases or examples to use the new APIs
Others
Merging options
Other information