Skip to content

Chart: Plugin optimization #6030

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

Merged
merged 8 commits into from
Mar 28, 2025
Merged

Chart: Plugin optimization #6030

merged 8 commits into from
Mar 28, 2025

Conversation

tesar-tech
Copy link
Collaborator

Description

(originally in #6028)

  • Puts common logic and properties into ChartPlugin.
  • Solves pain-points of async eventing between chart and plugins.
  • Changes how plugins are connected to the BaseChartOfT
  • Adds few comments about the connection

This optimization will reduce the necessary code in every plugin significantly, can solve the disposing on one place in initializing on one place. Potential bugs will be solved for all of the plugins at one place.

  • Trendline plugin is different - I added comments inside the ChartTrendline.razor.cs

To answer the questions from previous pr:

Why Trendline doesn't have the DisposeAsync implementation ?

I think it is just missing the same disposal logic as the other plugins, it's about the await JSModule.SafeDisposeAsync(); that is missing.

Why Streaming doesn't have the OnParameterSet implementation?

Part of the "refresh" strategy is done in Refresh method, but the parameters Vertical and Options wont't get there. They are used only in plugin initialization. Thus I think, the OnParameterSet should have its implementation. But maybe I am missing something.

@tesar-tech tesar-tech requested a review from stsrki March 19, 2025 13:55
@tesar-tech tesar-tech marked this pull request as ready for review March 19, 2025 13:55
Copy link
Collaborator

@stsrki stsrki left a comment

Choose a reason for hiding this comment

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

Looking good. I made some changes on the method names and structure of the code, but overall the principle is the same.

If there is any need update the docs. And also add release notes.

@stsrki stsrki mentioned this pull request Mar 25, 2025
@stsrki stsrki merged commit df7c1ca into master Mar 28, 2025
2 checks passed
@stsrki stsrki deleted the dev/chart-plugins-improvements branch March 28, 2025 08:57
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants