Echart enhancements - onclick and dynamic options

[thetableman]

This PR replaces #1562 and proposes dynamic option parsing in addition to onclick callback.

[thetableman]

@natankeddem, regarding your comments here: #1562 (comment) it seems you already have a good start on understanding how these tests should proceed - perhaps if you continue in this space while I look at developing demos?

[natankeddem]

Yes, I can continue coming up with a test plan. Although, as described before we probably need to talk about how we want to go about it due to to way this library renders its contents. @rodja @falkoschindler any input on how you would like to process purely graphical tests would be helpful. Perhaps I can capture golden images of the chart rendering to compare against or something similar?

Before I get there I just wanted to take tonight to review what you have already implemented.

[rodja]

@rodja @falkoschindler any input on how you would like to process purely graphical tests would be helpful.

That's really tricky. We have tried to avoid these kind of tests until know. For example the ui.scene tests are more javascript query tests than really checking the rendering. I was hoping something like that would also be enough for ui.echart. The philosophy is like this:
Element tests should ensure that the wrapping and bridging of the js library works properly. We do not need to test the rendering itself because this is already ensured by the developers of the js library.

[natankeddem]

Okay, I will try to limit testing to checking the superficial element div/canvas rather than trying to interact directly with the internal rendering for now and see what we can get done.

[natankeddem]

I added a few tests to start things off. The expansion test will fail until #1570 hopefully fixes that issue. I still haven't gotten time to play around with the events. Do you have any test charts you have been playing around with this event that you could share?

[thetableman]

Yes, I've started the demo, it's just taken a while to understand how it's all set up. However I'm now coming across an issue with dynamic_properties.js, multiple browsers are refusing to load the page because of a mime type issue.

Any thoughts on what the problem could be?

[rodja]

@thetableman the mime-type issue looks familiar. Maybe it's #1510?

[thetableman]

@thetableman the mime-type issue looks familiar. Maybe it's #1510?

Yes, it's probably related. I didn't have an issue at work yesterday where I run 1.3.13 on Win 10 in Firefox but this was at home on Win 11 (but still 1.3.13 and Firefox). Not sure if the os is relevant though as the error suggests it's to do with the js loading in a browser. I'm not experienced with JavaScript so this one is definitely beyond my understanding.

[natankeddem]

If you point your browser toward dynamic_properties.js manually in the url bar does it load?

[natankeddem]

Can you try adding the following at the top of your python script you are running before any other code:

import mimetypes
mimetypes.add_type('application/javascript', '.js')
mimetypes.add_type('text/css', '.css')

[thetableman]

If you point your browser toward dynamic_properties.js manually in the url bar does it load?

Yes, the javascript is accessible in the browser. The error isn't related to be able to find the resource but to do with importing it.

[thetableman]

Can you try adding the following at the top of your python script you are running before any other code:

import mimetypes
mimetypes.add_type('application/javascript', '.js')
mimetypes.add_type('text/css', '.css')

Adding to the top of the running script had no effect but after adding to the top of the ui.echart script the console error disappeared and the chart loaded.

[natankeddem]

Can you try adding the following at the top of your python script you are running before any other code:

import mimetypes
mimetypes.add_type('application/javascript', '.js')
mimetypes.add_type('text/css', '.css')

Adding to the top of the running script had no effect but after adding to the top of the ui.echart script the console error disappeared and the chart loaded.

I am not sure I understand; can you be more specific on where you added that code to make things work?

If that did in fact fix things; there might need to be a seperate PR to fix the issue. It shouldn't be specific to echarts and should affect any imports it appears. The way I understand it is there is some sort of Windows registry mime type usage information and somehow that makes things not work on random Windows computers. I didn't read into the nitty gritty to be honest. Here is something Django specific with some informative links. It appeared to be a widespread problem that causes issues in quite a few utilities/libraries in several different languages. https://www.django-unicorn.com/docs/troubleshooting/

[thetableman]

You suggested I add the mimetypes to the top of the script I'm running, which was an example for the demo I was working on - this didn't resolve the issue. Instead I added the changes to the ui.echart element script - this did seem to resolve the issue.

I've just done another commit with the change.

[natankeddem]

Alright, I didn't realize what you were working on. Can you try removing it from echart.py and add it to the top of main.py(are you running this for docs demo or something else?). Make sure all the code I posted is above any other imports, hopefully autopep8 allows this. Make sure it isn't split as in the current echarts.py. This should give more data to whomever fixes this in the future. Hopefully this code can be added somewhere just once not in every element that needs to use imports.

[thetableman]

Alright, I didn't realize what you were working on. Can you try removing it from echart.py and add it to the top of main.py(are you running this for docs demo or something else?). Make sure all the code I posted is above any other imports, hopefully autopep8 allows this. Make sure it isn't split as in the current echarts.py. This should give more data to whomever fixes this in the future. Hopefully this code can be added somewhere just once not in every element that needs to use imports.

I came across the error whilst testing some demo scenarios so it seemed logical to just continue testing this change with them. I'll try placing the code in a main.py tomorrow but I need to put a pin in this today as I have other plans.

[natankeddem]

I'm thinking it will eventually probably need to go on the top of nicegui\ui.py or something like that but I'm thinking figuring out exactly where would be a job for @rodja or @falkoschindler with some more experience...
Enjoy your night!

[thetableman]

I'm thinking it will eventually probably need to go on the top of nicegui\ui.py or something like that but I'm thinking figuring out exactly where would be a job for @rodja or @falkoschindler with some more experience...
Enjoy your night!

Agree, my gut feeling is the mimetype will need to be declared before the dynamic_properties.js script is loaded, doing so after from nicegui import ui means the ui.echart element has already been imported and established as a class including that js script.

Also agree it may be best to find somewhere more global for the mimetypes so that other elements benefit.

[rodja]

Thanks for digging into this @thetableman and @natankeddem. This kind of errors are hard to track down. It would be really great to have the fix in a separate PR. Maybe __init__.py in the nicegui main directory would be a good place? It's loaded before everything else.

[thetableman]

Thanks for digging into this @thetableman and @natankeddem. This kind of errors are hard to track down. It would be really great to have the fix in a separate PR. Maybe __init__.py in the nicegui main directory would be a good place? It's loaded before everything else.

PR #1594 has been submitted.

[falkoschindler]

Thanks for this PR, @thetableman!
I looked it through and updated the code here and there:

• I removed the pack() function, since NiceGUI automatically transmits the selected event arguments to the server. There's no need to rename them here.
• Fixing the mimetypes is already on main. No need to do it here.
• ui.chart and ui.echart are separate UI elements. I don't think there event classes should derive from another.
• And I cleanup tests and documentation. Especially the demos could be shortened a bit to really focus on a single feature each.

I guess it's ready to merge on main. 😀