-
Notifications
You must be signed in to change notification settings - Fork 12
feat: emit event on legend highlight exit #131
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
feat: emit event on legend highlight exit #131
Conversation
| /** | ||
| * Called when a legend item highlight is cleared. | ||
| */ | ||
| onLegendItemHighlightExit?: NonCancelableEventHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the same be added to the CoreLegendProps?
Also, how is that different from the existing onClearHighlight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the same be added to the CoreLegendProps?
Nice catch, I see thatonLegendItemHighlightis also not there, so I'll add both of those.
Also, how is that different from the existing onClearHighlight?
This occurs after we move the mouse away from the legend and is emitted by legend, onClearHighlight is called once we move the mouse away from the series in the graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HrvojeHemen, would it make sense to call the onClearHighlight also when the legend item highlight is lost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is CoreLegend onItemHighlight and onClearHighlight are different than the new onLegendItemHighlight and onLegendItemHighlightExit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HrvojeHemen, would it make sense to call the onClearHighlight also when the legend item highlight is lost?
I don't want to couple the events, if needed the user can call it themselves.
We should support both useCases where someone wants to also clearHighlight and doesn't, calling it on legendItemHighlightExit would stop this from being backwards compatible as existing graphs would start getting existing events emitted from a different source.
How is CoreLegend onItemHighlight and onClearHighlight are different than the new onLegendItemHighlight and onLegendItemHighlightExit?
Explained in another comment, https://github.com/cloudscape-design/chart-components/pull/131/files#r2598547502
1d52986 to
7c5a5bd
Compare
| @@ -0,0 +1,145 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page only covers the standalone legend use case, but not charts.
How about we create a page that:
- Has a standalone legend and a two charts where each has a legend, too
- The events between all legends are synced
This page can be then used as a playground to see why exactly the existing event listeners that we have in the chart and in the legend at not enough (if that is the case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think thats needed. This was a demo page to show the usecase of emission of exit events.
This doesn't affect other charts currently as it is not being consumed in this project, but externally, we just needed a way to catch that event.
If needed I can make a small POC sandbox with two charts that are fully event synced, but I don't think that is needed for this PR.
src/core/interfaces.ts
Outdated
| onItemHighlight?: NonCancelableEventHandler<CoreLegendProps.ItemHighlightDetail>; | ||
| onVisibleItemsChange?: NonCancelableEventHandler<CoreLegendProps.VisibleItemsChangeDetail>; | ||
| onLegendItemHighlight?: NonCancelableEventHandler<CoreChartProps.LegendItemHighlightDetail>; | ||
| onLegendItemHighlightExit?: NonCancelableEventHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These onLegendItemHighlight and onLegendItemHighlightExit are unused. Did you mean to use them in CoreLegend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops my bad, I will hook them up properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually going back to this, legend itself doesn't use onLegendItemHighlight and onLegendItemHighlightExit.
Those props are used on chart, legend uses onItemHighlight and onClearHighlight
| title="Master Legend (Has Exit Handler)" | ||
| ariaLabel="Legend with exit handler that controls both legends" | ||
| onItemHighlight={handleLegendItemHighlightWithExit} | ||
| onClearHighlight={handleLegendItemHighlightExitWithExit} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses the existing onClearHighlight. Did you mean to showcase this or the new onLegendItemHighlightExit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at CoreChart, you will see that it passes onLegendItemHighlightExit and onLegendItemHighlight to CoreLegend through onItemHighlight and onClearHighlight.
I think that the usage of onLegendItemHighlight on chart turning into onItemHighlight was done that way because if the prop is on legend, there is no need to add legend substring into the variable.
So current flow, before my PR:
Chart gets a prop called onLegendItemHighlight and passes it as onItemHighlight to the tooltip.
I simply added another prop for onLegendItemHighlightExit and passed it the same way.
So to answer your question, I am testing the new feature. If it's confusing I can refactor the props and rename them to keep their naming across both the chart and legend.
7c5a5bd to
e11dc93
Compare
|
Cancelled in favor of a much simpler approach, #136 |
Description
We want to emit events on legend item exit so that we can properly event sync the graphs, current implementation doesn't allow legend highlight clear propagation to other graphs
Use case
We have a dashboard with multiple charts, each of those charts have a legend of their own.
All charts have same named metrics, displayed with different math or formatting applied onto them.
Clicking or hovering on a series on any of those charts should control the behaviour of other charts, a term we call event syncing of charts.
The issue is that legend highlight exit listener is not getting emitted, so while we can detect legend click, and legend hover, we cannot detect a legend hover exit.
This brings us to the issue:
If we hover over the legend entry of metric A on chart A, metric A will be highlighted on chart A and chart B.
If we then remove the mouse from metric A away from the chart, chart A will update itself to the expected state as there is no need for events inside the charts, but other charts that are listening to the events will not know that the mouse moved away from the legend as there is no event, and will therefore stay selected.
How has this been tested?
Snap tests, manual testing, storybook
Before, without the event handler:
2025-12-05_15-23-54.mp4
After, with the event handler
2025-12-05_15-27-08.mp4
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.