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] Allow passing of DOM nodes to setContent #13469

Merged
merged 2 commits into from Oct 26, 2020
Merged

[5.0] Allow passing of DOM nodes to setContent #13469

merged 2 commits into from Oct 26, 2020

Conversation

Ghostbird
Copy link
Contributor

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Allows Tooltip to be set with array of DOM Nodes instead of HTML string.

Fixed issues

Details

Before: What was the problem?

When the tooltip is set, you could only pass static HTML content. I am developing an Angular app. Angular – like other web frameworks (e.g Vue, React) – has its own template mechanism to generate DOM nodes and retain control over their contents, to update them dynamically. However, I could not leverage my framework inside echarts tooltip.

After: How is it fixed in this PR?

A non-framework approach is shown in the test file tooltip-domnode.html. That tooltip shows the continuously updating current date.
In my Angular project. I can return this.viewContainerRef.createEmbeddedView(template, context).rootNodes from the EChartOption.Tooltip.Formatter and my tooltip content will be updated in Angular's lifecycle mechanism.

Usage

Are there any API changes?

  • The API has been changed.

Previously this was the Formatter interface in Typescript:

interface Formatter {
    (params: Format | Format[], ticket: string, callback: (ticket: string, html: string) => void): string;
}

Now it is this:

interface Formatter {
    (params: Format | Format[], ticket: string, callback: (ticket: string, htmlOrDomNodes: string | any[]) => void): string | any[];
}

Related test cases or examples to use the new APIs

  • tooltip-domnode.html

Others

Merging options

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

Other information

@pissang
Copy link
Contributor

pissang commented Oct 22, 2020

Thanks for your contribution! Do you mind retargeting the PR to the next branch? It's our current developing branch and has been rewritten with typescript language.

Proposed implementation of #13250
Works well in my Angular project. I can return `ViewContainerRef.createEmbeddedView(template, context).rootNodes` from the `EChartOption.Tooltip.Formatter` and my tooltip content will be updated in Angular's lifecycle mechanism.
@Ghostbird Ghostbird changed the base branch from master to next October 22, 2020 07:37
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 22, 2020
@Ghostbird
Copy link
Contributor Author

Retargeted to next branch.

@pissang pissang merged commit 3041c9b into apache:next Oct 26, 2020
@pissang pissang changed the title Allow passing of DOM nodes to setContent [5.0] Allow passing of DOM nodes to setContent Oct 26, 2020
if (!isArray(content)) {
content = [content];
}
for (let i = 0; i < content.length; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up!
I'm curious, is a for loop with repeated indexing better than a for … of loop? I know that it is more memory efficient in compiled languages where arrays are implemented as contiguous areas of memory, but I had no idea the same was true for Javascript.

@Ghostbird Ghostbird deleted the fix-13250 branch October 27, 2020 16:19
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.

None yet

2 participants