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

echarts: simplify usage and remove current limitations #735

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mij
Copy link

@mij mij commented Sep 1, 2022

Hello folks,

The patch attached resolves, in a fully backwards-compatible way, the problem of echarts in LoveIt unable to support Javascript.

The patch supports this new form of using the shortcode:

{{< echarts file="charts/mychart.js" >}}{{< /echarts >}}

The shortcode expands this to a new attribute

<div class="echarts" ... data-filename="charts/mychart.js" ...>

and when the page is loaded, theme.js automatically recognizes which divs to load as a JS module vs with embedded specification, based on the presence of that data-filename attribute.

The documentation is updated accordingly, to list both ways of use: the "embedded" way (current), and the "referenced" way (with JS module).

As charts become non-trivial, the embedded way quickly become unusable, not only because it prevents using JavaScript which is essential to non-trivial charts, but also because the effort of converting specs back and forth from JavaScript to JSON/YAML/TOML when prototyping the chart with echart's editor becomes huge.

There are additional benefits of this approach:

  • Cleaner pages, not overbloating posts with specs code. Also doesn't break syntax highlighting of some browsers.
  • Trivially allows re-using a chart in multiple posts.
  • Improves speed of page loads by removing echarts code and allowing browser to cache JS file with it.

Nota bene: I haven't committed the auto-generate assets/js/theme.js.

Let me know if there's any additional adaptation you'd like to see in this.

@sonarcloud
Copy link

sonarcloud bot commented Sep 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mij mij mentioned this pull request Sep 1, 2022
@mij mij changed the title Simplify echarts support json echarts: simplify usage and remove current limitations Sep 25, 2022
@mij
Copy link
Author

mij commented Feb 18, 2023

Hello folks!

Is there any more input or code I could provide to encourage merging this patch?

A brief memo:

  • Fully backwards compatible.
  • Adds description of new feature in documentation.
  • Enables major functionality in using echarts, towards becoming must-have as echarts relies more on JavaScript expressions over time.

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

1 participant