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

Adding ECharts Element #1346

Merged
merged 13 commits into from
Aug 18, 2023
Merged

Adding ECharts Element #1346

merged 13 commits into from
Aug 18, 2023

Conversation

natankeddem
Copy link
Contributor

This is my first shot at adding ECharts(https://echarts.apache.org/) as a functional element. I mostly followed the lead from the existing chart element. ECharts has some weird sizing issues that might need to be resolved still.
for reference: https://stackoverflow.com/questions/33454946/echarts-and-bootstrap-responsive-width
Without setting the height/width it doesn't render correctly.

@PierreFaraut
Copy link

Hi @natankeddem,
Thanks a lot for your work here! Really appreciated!
Would it be possible to add a "on_click" event? To have a way to retrieve the point / data clicked by the user in the graph?
Thanks a lot!

@natankeddem
Copy link
Contributor Author

This was only planned as a minimal implementation. What you are asking for should be possible, since there are examples utilizing this sort of chart interaction:
https://echarts.apache.org/examples/en/editor.html?c=line-draggable
I will need to become more intimate with echarts before I can introduce this more advanced functionality. I will leave it up to the maintainers if they want expand this PR scope or push those features to another PR?

@PierreFaraut
Copy link

@natankeddem, I understand 👍
The work already done is already a super evolution in my opinion! We'll see if it needs to be improved in the future.
Thanks again!

@natankeddem
Copy link
Contributor Author

I don't know your use case, but if you can live with the existing charts (Highcharts) element I have added the point click functionality in a new PR.
#1350
I would appreciate any feedback on implementation since this isn't a feature that I have a specific need for at the moment.

@natankeddem
Copy link
Contributor Author

I did look into adding events to this implementation. It will be much more involved than the implementation for Highcharts. It would basically require the drawing of geometry on top of all the points then adding event callbacks tied to that geometry. I think this would mean that the handling on this would probably be very chart dependant, What works for a line chart wouldn't necessarily work for a bar for example.
An example of this implementation here:
https://echarts.apache.org/examples/en/editor.html?c=line-draggable

What are the thoughts from the maintainers on this element? Do we already have too many graphing/charting elements or is this worth adding? I didn't do much work on this other than the POC. I can flesh it out further if people want this.

@qingant
Copy link
Contributor

qingant commented Aug 17, 2023

@natankeddem this is a great thing to have in NiceGUI. Given the licensing policy of highcharts (ui.charts), it's actually NOT an option for a lot of projects, developer would need to move to other option when they suddenly found they are required to pay for it.

I think ECharts is at the same level as highcharts. Matplotlib and plotly are nice, but far from echarts/highcharts in very serious web data visualization area.

I am looking forward to see this being added to NiceGUI !

@natankeddem
Copy link
Contributor Author

Since there appears to be interest I did a bit more cleaning up and transitioned the example to the website documentation. I basically duplicated the existing chart element example which should hopefully show off the differences innate in each library inherently.
There is some issue with the "See more" link on the documentation that I need to look further into. It would be welcome if anyone could play around with the implementation I have so far to see if there is any issues I am missing.
For now I think this should be a minimal implementation with a door left open for additional PRs in the future to add advanced features like events.

@falkoschindler
Copy link
Contributor

@natankeddem First of all, thank you for this great PR! I finally found the time to dive into it.

I was just about to push some cleanup and refactoring I made during a code review. I'll try to merge with your recent changes.

@natankeddem
Copy link
Contributor Author

Thank you @falkoschindler ! This seemed like an easy enough start at a ground up element using chart as a starting point. Admittedly I don't know all intricacies of NiceGUI with regards to implementing everything and really appreciate you helping me along.

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

That's a very nice implementation to start with this new UI element. I just improved a few things here and there:

  • I updated npm.json so that dependencies are loaded using our npm.py script.
  • I removed unused versions to reduce the number of dependencies served.
  • I also removed the extensions API because it wasn't functional (typo: "extension" vs. "extensions"), I didn't know how to use them (no demo etc.) and the npm.json would have gotten more complicated. Let's add them later if needed.
  • Similar to elements like ui.aggrid I added a default styling to nicegui.css. This way the user can easily overwrite it with .style and .classes.
  • I simplified the demo a bit. If we want to show more features, I'd suggest to create more demos below.

Now I just wonder if ui.echarts is the right name, or if we should rename it to ui.echart. I.e. "ECharts" is the library and "EChart" is the chart element. What do you think?

@falkoschindler falkoschindler added this to the 1.3.10 milestone Aug 17, 2023
@falkoschindler falkoschindler added the enhancement New feature or request label Aug 17, 2023
@natankeddem
Copy link
Contributor Author

Sorry, I didn't finish flesh out my POC sooner; it looks like we duplicated some efforts. Good to see I wasn't too far off with my changes I made in parallel with you. Are you sure you don't like my new example? I thought it would be interesting to show both elements displaying a similar sort of dataset and functionality on the main page. If it is too complex I understand.

I know what you mean with the naming it doesn't seem correct, I only kept it consistent with "echarts" since that is the library name and minimized what I need to think about. I am good with either name; I have no preference. Should I submit the change in naming or would you like to? I am not sure which names should change/not change to align with your preferences on this possibly.

@falkoschindler
Copy link
Contributor

@natankeddem No worries, the merge conflict wasn't too bad.
I just renamed ui.echarts to ui.echart.
And you're right, using the same demo for both elements is a good idea. So I switched back to your bar chart.
I think we're ready to merge! 😀

@falkoschindler falkoschindler merged commit 68499ec into zauberzeug:main Aug 18, 2023
1 check passed
@rodja rodja mentioned this pull request Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants