-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { useMemo, useState } from "react"; | ||
|
|
||
| import Alert from "@cloudscape-design/components/alert"; | ||
| import Badge from "@cloudscape-design/components/badge"; | ||
| import Box from "@cloudscape-design/components/box"; | ||
| import SpaceBetween from "@cloudscape-design/components/space-between"; | ||
|
|
||
| import { CoreLegend } from "../../lib/components/internal-do-not-use/core-legend"; | ||
| import { Page } from "../common/templates"; | ||
|
|
||
| // Base legend items data | ||
| const baseLegendItems = [ | ||
| { | ||
| id: "series-a", | ||
| name: "Series A", | ||
| marker: <div style={{ width: 12, height: 12, backgroundColor: "#1f77b4", borderRadius: 2 }} />, | ||
| visible: true, | ||
| }, | ||
| { | ||
| id: "series-b", | ||
| name: "Series B", | ||
| marker: <div style={{ width: 12, height: 12, backgroundColor: "#ff7f0e", borderRadius: 2 }} />, | ||
| visible: true, | ||
| }, | ||
| { | ||
| id: "series-c", | ||
| name: "Series C", | ||
| marker: <div style={{ width: 12, height: 12, backgroundColor: "#2ca02c", borderRadius: 2 }} />, | ||
| visible: true, | ||
| }, | ||
| ]; | ||
|
|
||
| export default function LegendEventsDemo() { | ||
| // State for tracking which item is highlighted in each legend | ||
| const [highlightedItemWithExit, setHighlightedItemWithExit] = useState<string | null>(null); | ||
| const [highlightedItemWithoutExit, setHighlightedItemWithoutExit] = useState<string | null>(null); | ||
|
|
||
| // Create legend items with dynamic highlighted state for the legend WITH exit handler | ||
| const legendItemsWithExit = useMemo( | ||
| () => | ||
| baseLegendItems.map((item) => ({ | ||
| ...item, | ||
| highlighted: item.name === highlightedItemWithExit, | ||
| })), | ||
| [highlightedItemWithExit], | ||
| ); | ||
|
|
||
| // Create legend items with dynamic highlighted state for the legend WITHOUT exit handler | ||
| const legendItemsWithoutExit = useMemo( | ||
| () => | ||
| baseLegendItems.map((item) => ({ | ||
| ...item, | ||
| highlighted: item.name === highlightedItemWithoutExit, | ||
| })), | ||
| [highlightedItemWithoutExit], | ||
| ); | ||
|
|
||
| // Handler for legend WITH exit handler - controls both legends | ||
| const handleLegendItemHighlightWithExit = (event: { detail: { item: { name: string } } }) => { | ||
| const itemName = event.detail.item.name; | ||
| setHighlightedItemWithExit(itemName); | ||
| // Cross-control: also highlight the corresponding item in the other legend | ||
| setHighlightedItemWithoutExit(itemName); | ||
| }; | ||
|
|
||
| // Handler for exit event - only clears the legend WITH exit handler | ||
| const handleLegendItemHighlightExitWithExit = () => { | ||
| setHighlightedItemWithExit(null); | ||
| // Cross-control: also clear the other legend (this shows proper bidirectional control) | ||
| setHighlightedItemWithoutExit(null); | ||
| }; | ||
|
|
||
| // Handler for legend WITHOUT exit handler - controls both legends | ||
| const handleLegendItemHighlightWithoutExit = (event: { detail: { item: { name: string } } }) => { | ||
| const itemName = event.detail.item.name; | ||
| setHighlightedItemWithoutExit(itemName); | ||
| // Cross-control: also highlight the corresponding item in the other legend | ||
| setHighlightedItemWithExit(itemName); | ||
| }; | ||
|
|
||
| // Note: No exit handler for the second legend - this demonstrates the issue | ||
|
|
||
| return ( | ||
| <Page | ||
| title="Legend Events" | ||
| subtitle="Demonstrates the difference between having onLegendItemHighlightExit and not having it" | ||
| > | ||
| <SpaceBetween direction="vertical" size="l"> | ||
| {/* Legend WITH exit handler */} | ||
| <Box> | ||
| <SpaceBetween direction="vertical" size="m"> | ||
| <SpaceBetween direction="horizontal" size="s" alignItems="center"> | ||
| <Box variant="h2">Legend WITH onClearHighlight (Controls Both)</Box> | ||
| <Badge color={highlightedItemWithExit ? "blue" : "grey"}> | ||
| {highlightedItemWithExit ? `Controlling: ${highlightedItemWithExit}` : "Not Controlling"} | ||
| </Badge> | ||
| </SpaceBetween> | ||
|
|
||
| <CoreLegend | ||
| items={legendItemsWithExit} | ||
| title="Master Legend (Has Exit Handler)" | ||
| ariaLabel="Legend with exit handler that controls both legends" | ||
| onItemHighlight={handleLegendItemHighlightWithExit} | ||
| onClearHighlight={handleLegendItemHighlightExitWithExit} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This uses the existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you look at CoreChart, you will see that it passes I think that the usage of So current flow, before my PR: Chart gets a prop called I simply added another prop for 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. |
||
| /> | ||
| </SpaceBetween> | ||
| </Box> | ||
|
|
||
| {/* Legend WITHOUT exit handler */} | ||
| <Box> | ||
| <SpaceBetween direction="vertical" size="m"> | ||
| <SpaceBetween direction="horizontal" size="s" alignItems="center"> | ||
| <Box variant="h2">Legend WITHOUT onClearHighlight (Controls Both)</Box> | ||
| <Badge color={highlightedItemWithoutExit ? "red" : "grey"}> | ||
| {highlightedItemWithoutExit ? `Stuck Controlling: ${highlightedItemWithoutExit}` : "Not Controlling"} | ||
| </Badge> | ||
| </SpaceBetween> | ||
|
|
||
| <CoreLegend | ||
| items={legendItemsWithoutExit} | ||
| title="Broken Legend (No Exit Handler)" | ||
| ariaLabel="Legend without exit handler that controls both legends" | ||
| onItemHighlight={handleLegendItemHighlightWithoutExit} | ||
| // Note: No onClearHighlight prop - this demonstrates the issue | ||
| /> | ||
| </SpaceBetween> | ||
| </Box> | ||
|
|
||
| <Alert type="success"> | ||
| <strong>Expected Behavior:</strong> | ||
| <br />• <strong>Top Legend (With Exit Handler):</strong> When you hover and then move away, both legends clear | ||
| their highlights properly - demonstrating proper bidirectional control. | ||
| <br />• <strong>Bottom Legend (Without Exit Handler):</strong> When you hover and then move away, both legends | ||
| remain stuck in the highlighted state - demonstrating the broken one-way control that the exit handler fixes. | ||
| <br /> | ||
| <br /> | ||
| This shows how the exit handler is essential for proper cross-component communication and state management. | ||
| </Alert> | ||
| </SpaceBetween> | ||
| </Page> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { act } from "react"; | ||
| import highcharts from "highcharts"; | ||
| import { vi } from "vitest"; | ||
|
|
||
| import { KeyCode } from "@cloudscape-design/component-toolkit/internal"; | ||
|
|
||
| import { createChartWrapper, renderChart } from "./common"; | ||
|
|
||
| import legendTestClasses from "../../../lib/components/internal/components/chart-legend/test-classes/styles.selectors.js"; | ||
|
|
||
| const series: Highcharts.SeriesOptionsType[] = [ | ||
| { | ||
| type: "line", | ||
| name: "L1", | ||
| data: [1], | ||
| }, | ||
| { | ||
| type: "line", | ||
| name: "L2", | ||
| data: [2], | ||
| }, | ||
| { | ||
| type: "line", | ||
| id: "L3", | ||
| name: "Line 3", | ||
| data: [3], | ||
| }, | ||
| ]; | ||
|
|
||
| const getItemSelector = (options?: { active?: boolean; dimmed?: boolean }) => { | ||
| let selector = `.${legendTestClasses.item}`; | ||
| if (options?.active === true) { | ||
| selector += `:not(.${legendTestClasses["hidden-item"]})`; | ||
| } | ||
| if (options?.active === false) { | ||
| selector += `.${legendTestClasses["hidden-item"]}`; | ||
| } | ||
| if (options?.dimmed === true) { | ||
| selector += `.${legendTestClasses["dimmed-item"]}`; | ||
| } | ||
| if (options?.dimmed === false) { | ||
| selector += `:not(.${legendTestClasses["dimmed-item"]})`; | ||
| } | ||
| return selector; | ||
| }; | ||
|
|
||
| const getItem = (index: number, options?: { active?: boolean; dimmed?: boolean }) => | ||
| createChartWrapper().findLegend()!.findAll(getItemSelector(options))[index]; | ||
|
|
||
| const mouseOver = (element: HTMLElement) => element.dispatchEvent(new MouseEvent("mouseover", { bubbles: true })); | ||
| const mouseOut = (element: HTMLElement) => element.dispatchEvent(new MouseEvent("mouseout", { bubbles: true })); | ||
| const clearHighlightPause = () => new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| describe("CoreChart: legend events", () => { | ||
| test("calls onLegendItemHighlightExit when leaving a legend item", async () => { | ||
| const onLegendItemHighlightExit = vi.fn(); | ||
| renderChart({ highcharts, options: { series }, onLegendItemHighlightExit }); | ||
|
|
||
| // Hover over a legend item first | ||
| act(() => mouseOver(getItem(0).getElement())); | ||
| expect(onLegendItemHighlightExit).not.toHaveBeenCalled(); | ||
|
|
||
| // Leave the legend item | ||
| act(() => mouseOut(getItem(0).getElement())); | ||
| await clearHighlightPause(); | ||
|
|
||
| expect(onLegendItemHighlightExit).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("calls onLegendItemHighlightExit when pressing escape on a focused legend item", () => { | ||
| const onLegendItemHighlightExit = vi.fn(); | ||
| renderChart({ highcharts, options: { series }, onLegendItemHighlightExit }); | ||
|
|
||
| // Focus on a legend item first | ||
| getItem(0).focus(); | ||
| expect(onLegendItemHighlightExit).not.toHaveBeenCalled(); | ||
|
|
||
| // Press escape to clear highlight | ||
| getItem(0).keydown({ keyCode: KeyCode.escape }); | ||
|
|
||
| expect(onLegendItemHighlightExit).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("calls onLegendItemHighlightExit only once when multiple legend items are involved", async () => { | ||
| const onLegendItemHighlightExit = vi.fn(); | ||
| renderChart({ highcharts, options: { series }, onLegendItemHighlightExit }); | ||
|
|
||
| // Hover over first legend item | ||
| act(() => mouseOver(getItem(0).getElement())); | ||
| expect(onLegendItemHighlightExit).not.toHaveBeenCalled(); | ||
|
|
||
| // Move to second legend item (should not trigger onLegendItemHighlightExit) | ||
| act(() => mouseOut(getItem(0).getElement())); | ||
| act(() => mouseOver(getItem(1).getElement())); | ||
| expect(onLegendItemHighlightExit).not.toHaveBeenCalled(); | ||
|
|
||
| // Leave the second legend item (should trigger onLegendItemHighlightExit) | ||
| act(() => mouseOut(getItem(1).getElement())); | ||
| await clearHighlightPause(); | ||
|
|
||
| expect(onLegendItemHighlightExit).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("does not call onLegendItemHighlightExit when onLegendItemHighlightExit prop is not provided", async () => { | ||
| // This test ensures the component doesn't crash when the event handler is not provided | ||
| renderChart({ highcharts, options: { series } }); | ||
|
|
||
| // Hover over a legend item first | ||
| act(() => mouseOver(getItem(0).getElement())); | ||
|
|
||
| // Leave the legend item - should not crash | ||
| act(() => mouseOut(getItem(0).getElement())); | ||
| await clearHighlightPause(); | ||
|
|
||
| // Test passes if no error is thrown | ||
| expect(true).toBe(true); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -368,6 +368,10 @@ export interface CoreChartProps | |
| * Called when a legend item is highlighted. | ||
| */ | ||
| onLegendItemHighlight?: NonCancelableEventHandler<CoreChartProps.LegendItemHighlightDetail>; | ||
| /** | ||
| * Called when a legend item highlight is cleared. | ||
| */ | ||
| onLegendItemHighlightExit?: NonCancelableEventHandler; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't want to couple the events, if needed the user can call it themselves.
Explained in another comment, https://github.com/cloudscape-design/chart-components/pull/131/files#r2598547502 |
||
| /** | ||
| * Called when series/points visibility changes due to user interaction with legend or filter. | ||
| */ | ||
|
|
||
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:
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.