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

enh/3957-legend-item-click-event #21096

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

Conversation

hubertkozik
Copy link
Member

@hubertkozik hubertkozik commented Apr 30, 2024

Introduced new event and option legend.events.itemClick, deprecated events.legendItemClick, closes #3957.


I am not sure if we should treat it as a Feature Request (label), but we don't have an Ehnacement label.


@hubertkozik hubertkozik added Product: Highcharts Type: Feature Request Used when a new feature is requested either directly or indirectly labels Apr 30, 2024
@hubertkozik hubertkozik self-assigned this Apr 30, 2024
@highsoft-bot
Copy link
Collaborator

Visual test results - No difference found


Samples changed

Change type Sample
Added samples/highcharts/legend/pie-legend-itemclick/demo.js
Added samples/highcharts/legend/series-legend-itemclick/demo.js
Deleted samples/highcharts/plotoptions/pie-point-events-legenditemclick/demo.js
Deleted samples/highcharts/plotoptions/series-events-legenditemclick/demo.js

@hubertkozik hubertkozik force-pushed the enh/3957-legend-item-click-event branch from d02bd91 to 8d361a2 Compare April 30, 2024 13:49
@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Apr 30, 2024

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
highcharts.js 95.8 kB
270.2 kB
95.7 kB
270.4 kB
-56 B
221 B
highstock.js 128.6 kB
371.9 kB
128.6 kB
372.1 kB
-52 B
221 B
highmaps.js 121.6 kB
349.5 kB
121.6 kB
349.7 kB
-58 B
198 B
highcharts-gantt.js 133.8 kB
385.8 kB
133.7 kB
386.0 kB
-47 B
221 B
dashboards/dashboards.js 43.1 kB
148.4 kB
43.1 kB
148.4 kB
4 B
10 B
datagrid/datagrid.js 24.7 kB
76.8 kB
24.7 kB
76.8 kB
4 B
10 B

Copy link
Contributor

@bre1470 bre1470 left a comment

Choose a reason for hiding this comment

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

Great work! Looks good. 👍

@hubertkozik hubertkozik added Changelog: Feature Use on PR to add description as a feature in the generated changelog. and removed Type: Feature Request Used when a new feature is requested either directly or indirectly labels May 2, 2024
@hubertkozik hubertkozik requested review from KacperMadej and removed request for KacperMadej May 6, 2024 14:22
Copy link

@KacperMadej KacperMadej left a comment

Choose a reason for hiding this comment

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

Could you add the version number after the added deprecated tags? Using @next should work fine.

@bre1470
Copy link
Contributor

bre1470 commented May 8, 2024

Could you add the version number after the added deprecated tags? Using @next should work fine.

Good idea, but this will not work here as these are not API options. JSDoc and Docstrap do not understand extended @deprecate tags. Kacper has to implement it first. 😄

@hubertkozik
Copy link
Member Author

We're using deprecated with version number, e.g. here: https://api.highcharts.com/highcharts/lang.accessibility.resetZoomButton I think it's worth adding it like Kacper suggested.

@hubertkozik hubertkozik marked this pull request as ready for review May 10, 2024 09:23
@bre1470
Copy link
Contributor

bre1470 commented May 13, 2024

This is not supported by JSDoc. Kacper is working on the new class reference, where it can be supported. Only the API options support the extended @deprecated tag.

Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

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

Thanks, great work!

ts/Core/Defaults.ts Outdated Show resolved Hide resolved
ts/Core/Legend/Legend.ts Outdated Show resolved Hide resolved
Comment on lines +1651 to +1653
if ((item as any).setVisible) {
(item as any).setVisible();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if ((item as any).setVisible) {
(item as any).setVisible();
}
(item as Point|Series).setVisible?.();

Trying to get rid of any. I'm not 100% sure this change is correct, can you check that?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can also be an item as a DataClass legend item, but we don't have good typing for this. Also, the item can be BubbleLegendItem, but this item does not contain setVisible method. Removing this any would need a lot of refactoring, so I left as any here. Do you think this refactor should be done now or we can leave it for this moment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Feature Use on PR to add description as a feature in the generated changelog. Product: Highcharts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

colorAxis with dataClasses does not allow legendItemClick
5 participants