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 onclick #1562

Closed
wants to merge 4 commits into from
Closed

Echarts onclick #1562

wants to merge 4 commits into from

Conversation

thetableman
Copy link
Contributor

This PR adds the on_click_point functionality to Apache Echarts.

this.chart.setOption(this.options);
this.chart.resize();
function unpack(e) {
Copy link
Member

Choose a reason for hiding this comment

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

Why have you picked this subset of event arguments? Have you seen the uncycle function in chart.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I could see in the documentation these are the options available in the response from the click event.
I saw the uncycle in chart.js but I'm not great with javascript and didn't understand it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we will let @falkoschindler decide which variant we will integrate. Thanks!

@rodja rodja added this to the 1.3.14 milestone Sep 6, 2023
@rodja rodja added the enhancement New feature or request label Sep 6, 2023
@natankeddem
Copy link
Contributor

@rodja
Should we close out my PR #1556 ?
This appears to add what I was going to do and then some more ancillary functionality.

@thetableman
Copy link
Contributor Author

Don't want to steal you're thunder @natankeddem, I just had your code in my repo for testing when I committed my onclick PR.

@natankeddem
Copy link
Contributor

No worries, I wanted to look further into the element testing and some other things. If you want to continue with this it will give me some time to do that. I will close my PR.

@rodja
Copy link
Member

rodja commented Sep 7, 2023

I'm also sorry @natankeddem. I haven't seen that this PR also includes the changes from #1556. But the same remarks I made in #1556 apply here: We will need tests and demos to the docs. I'll convert this to "Draft" for now. Maybe its simpler to reopen #1556 and let @thetableman create a new PR without the dynamic option conversion so we can work on both features separately?

Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

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

We need demos and tests.

@rodja rodja marked this pull request as draft September 7, 2023 03:19
@thetableman
Copy link
Contributor Author

We need demos and tests.

@rodja I can't find any tests for echarts at all in the main branch, we're any completed with the initial element PR?

@rodja
Copy link
Member

rodja commented Sep 7, 2023

You are right @thetableman. There where no tests added in #1346. It somehow slipped through. Maybe you can coordinate with @natankeddem? https://github.com/zauberzeug/nicegui/blob/main/tests/test_chart.py could be used as an inspiration.

@thetableman
Copy link
Contributor Author

Yes, I was thinking of basing it on the test for ui.chart but it too is missing tests for the point click. I'm also struggling a bit to identify the css of the visual created by the ui.echart element - eg the css for a bar.

@natankeddem
Copy link
Contributor

@thetableman Do you mind if we work together under your PR? You could add me as a collaborator. Since our contributions are interrelated it would probably be easier to manage. Also, it should be less heartburn on the maintainers with merging.
I suggest we concentrate on the following here, although I know echart has other issues:

  • Implement dynamic parsing for options.
  • Figure out the events we want and how to structure them.
  • Implement full testing suite.
  • Implement examples for any new functionality.

@natankeddem
Copy link
Contributor

natankeddem commented Sep 7, 2023

At first glance testing this element might be quite different than testing the chart element. echart utilizes a canvas element which makes extracting data more difficult than parsing the html that chart creates. This is my first major dive into this sort of testing so I need to research some more on this topic. This apache/echarts#13635 (comment) provides some insight into testing ECharts. It appears to boil down to needing to do graphical analysis of the rendered chart.

@thetableman
Copy link
Contributor Author

thetableman commented Sep 7, 2023

@thetableman Do you mind if we work together under your PR? You could add me as a collaborator. Since our contributions are interrelated it would probably be easier to manage. Also, it should be less heartburn on the maintainers with merging. I suggest we concentrate on the following here, although I know echart has other issues:

* Implement dynamic parsing for options.

* Figure out the events we want and how to structure them.

* Implement full testing suite.

* Implement examples for any new functionality.

Sounds good @natankeddem!

I'll have to close this PR and submit another because the repo is located in an organization and I can't invite external collaborators, I'll invite you from a personal repo instead.

This will be an opportunity to reference the dynamic parsing as part of the new PR too.

New PR is #1579

@falkoschindler
Copy link
Contributor

At first glance testing this element might be quite different than testing the chart element. echart utilizes a canvas element which makes extracting data more difficult than parsing the html that chart creates.

That's why I didn't insist on tests for #1346. But sure, it's always better to have them.

@falkoschindler falkoschindler removed this from the 1.3.14 milestone Sep 18, 2023
@thetableman thetableman deleted the echarts_onclick branch December 11, 2023 04:48
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.

4 participants