pawarprasad123 commented on code in PR #703:
URL: https://github.com/apache/atlas/pull/703#discussion_r3900885493
##########
dashboard/src/views/DashboardOverview/EntityStatusDonut.tsx:
##########
@@ -147,19 +106,20 @@ const EntityStatusDonut = memo(({ entity, isLoading }:
EntityStatusDonutProps) =
isAnimationActive
animationDuration={800}
animationEasing="ease-out"
-
activeIndex={activeIndex}
-
activeShape={renderActiveShape}
- onMouseEnter={(_,
index) => setActiveIndex(index)}
- onMouseLeave={() =>
setActiveIndex(-1)}
- onClick={(data) =>
handleStatusClick(data.name as "Active" | "Shell" | "Deleted")}
+ onClick={(data:
unknown) => {
Review Comment:
Pie onClick uses inline type casting (data as { name?: string }) instead of
the shared getPayloadFromRechartsEvent helper introduced in this PR. Using the
helper would reduce duplication and align with ClassificationDistributionCard /
EntityTypeBarChart.
##########
dashboard/src/views/Statistics/__tests__/EntityStatsChart.test.tsx:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import React from "react";
+import { render, screen, fireEvent } from "@testing-library/react";
+import EntityStatsChart from "../EntityStatsChart";
+
+jest.mock("recharts", () => {
+ const OriginalRecharts = jest.requireActual("recharts");
+ return {
+ ...OriginalRecharts,
+ ResponsiveContainer: ({ children }: { children: React.ReactNode
}) => <div>{children}</div>,
+ AreaChart: ({ children }: { children: React.ReactNode }) =>
<div>{children}</div>,
+ Area: () => <div data-testid="area" />,
+ CartesianGrid: () => <div />,
+ XAxis: () => <div />,
+ YAxis: () => <div />,
+ Tooltip: () => <div />,
+ Legend: ({ content }: { content: () => React.ReactNode }) => {
+ const Content = content;
+ return <div data-testid="legend-mock">{Content ?
<Content /> : null}</div>;
+ },
+ };
+});
+
+describe("EntityStatsChart custom legend", () => {
+ const mockOnLegendClick = jest.fn();
+ const mockGetColorForKey = jest.fn((key: string) => {
+ if (key === "Active") return "blue";
+ if (key === "Deleted") return "red";
+ if (key === "Shell") return "orange";
+ return "black";
+ });
+
+ const defaultProps = {
+ chartData: [
+ { timestamp: 1600000000000, Active: 10, Deleted: 2,
Shell: 1 },
+ ],
+ chartMode: "stacked",
+ activeKeys: { Active: true, Deleted: false, Shell: true },
+ onLegendClick: mockOnLegendClick,
+ getColorForKey: mockGetColorForKey,
+ };
+
+ beforeEach(() => {
+ jest.clearAllMocks();
+ });
+
+ it("renders the custom legend with correct aria-labels and handles
click toggles", () => {
+ render(<EntityStatsChart {...defaultProps} />);
+
+ const activeLegend = screen.getByTestId("legend-Active");
+ const deletedLegend = screen.getByTestId("legend-Deleted");
+
+ expect(activeLegend).toHaveAttribute("aria-label", "Active");
+ expect(deletedLegend).toHaveAttribute("aria-label", "Deleted");
+
+ fireEvent.click(activeLegend);
+ expect(mockOnLegendClick).toHaveBeenCalledWith("Active");
+
+ fireEvent.click(deletedLegend);
+ expect(mockOnLegendClick).toHaveBeenCalledWith("Deleted");
+ });
+
+ it("applies active styling when key is active", () => {
+ render(<EntityStatsChart {...defaultProps} />);
+
+ const activeLegend = screen.getByTestId("legend-Active");
+ const typography =
activeLegend.querySelector(".legend-typography");
+ const colorBox =
activeLegend.querySelector(".legend-color-box");
+
+ expect(typography).toHaveClass("legend-active");
+ expect(typography).not.toHaveClass("legend-inactive");
+ expect(colorBox).toHaveStyle("background-color: blue");
+ });
+
+ it("applies inactive styling when key is inactive", () => {
+ render(<EntityStatsChart {...defaultProps} />);
+
+ const deletedLegend = screen.getByTestId("legend-Deleted");
+ const typography =
deletedLegend.querySelector(".legend-typography");
+ const colorBox =
deletedLegend.querySelector(".legend-color-box");
+
+ expect(typography).toHaveClass("legend-inactive");
+ expect(typography).not.toHaveClass("legend-active");
+ // The value is rgb equivalent for '#d3d3d3' or just string
depending on jsdom, but typically testing library normalizes it to rgb(211,
211, 211) or allows matching string.
+ // using string matching for '#d3d3d3'
+ expect(colorBox).toHaveStyle({ backgroundColor: "#d3d3d3" });
+ });
+});
Review Comment:
Consider adding a test for keyboard interaction on legend ButtonBase
(Enter/Space key) to verify the accessibility refactor, and a negative case
where onLegendClick is not called on invalid input.
##########
dashboard/src/utils/metricsUtils.ts:
##########
@@ -239,3 +239,13 @@ export const getClassificationDistribution = (
.sort((a, b) => b.count - a.count)
.slice(0, topN);
};
+
+export interface RechartsEventPayload<T> {
+ payload?: T;
+}
+
+export const getPayloadFromRechartsEvent = <T>(item: unknown): T | undefined
=> {
Review Comment:
getPayloadFromRechartsEvent is a good abstraction used across multiple chart
components. Please add unit tests covering: valid payload object,
null/undefined input, primitive input, and object without payload key. Also
consider using this helper in EntityStatusDonut.tsx (line 109) for consistency.
##########
dashboard/src/views/DashboardOverview/__tests__/ClassificationDistributionCard.test.tsx:
##########
@@ -110,4 +137,38 @@ describe('ClassificationDistributionCard', () => {
expect(visibleTextNodes?.length).toBe(1);
expect(visibleTextNodes?.[0]?.textContent).toBe(truncatedLongName);
});
+
+ it('navigates to classification search on valid bar click', async () =>
{
+ const user = userEvent.setup();
+ render(
+ <MemoryRouter>
+ <ClassificationDistributionCard tag={{}} />
+ </MemoryRouter>,
+ );
+
+ await user.click(screen.getByTestId('bar'));
+
expect(mockNavigateToClassificationSearch).toHaveBeenCalledWith(expect.anything(),
shortName);
+ });
+
+ it('ignores bar click when payload is invalid/missing', async () => {
+ const user = userEvent.setup();
+ render(
+ <MemoryRouter>
+ <ClassificationDistributionCard tag={{}} />
+ </MemoryRouter>,
+ );
+
+ mockNavigateToClassificationSearch.mockClear();
+ mockBarClickPayload = null;
+ await user.click(screen.getByTestId('bar'));
+
expect(mockNavigateToClassificationSearch).not.toHaveBeenCalled();
+
+ mockBarClickPayload = 'bad' as unknown;
+ await user.click(screen.getByTestId('bar'));
+
expect(mockNavigateToClassificationSearch).not.toHaveBeenCalled();
+
+ mockBarClickPayload = {};
+ await user.click(screen.getByTestId('bar'));
+
expect(mockNavigateToClassificationSearch).not.toHaveBeenCalled();
+ });
});
Review Comment:
EntityTypeBarChart.test.tsx covers Y-axis label click, keyboard Enter/Space,
and empty-label guard cases. Consider adding equivalent tests here for
handleLabelClick on the Y-axis custom tick renderer to maintain parity.
##########
dashboard/src/views/Statistics/EntityStatsChart.tsx:
##########
@@ -56,6 +58,37 @@ const EntityStatsChart = ({
onLegendClick,
getColorForKey,
}: EntityStatsChartProps) => {
+ const legendPayload = useMemo(() => {
+ return Object.keys(activeKeys).map((key) => ({
+ id: key,
+ value: key,
+ color: activeKeys[key as keyof ActiveKeys] === true ?
getColorForKey(key) : "#d3d3d3",
+ inactive: !activeKeys[key as keyof ActiveKeys],
+ }));
+ }, [activeKeys, getColorForKey]);
+
+ const renderLegend = useCallback(
+ () => (
+ <Stack direction="row" spacing={2}
justifyContent="center" mt={1}>
+ {legendPayload.map((entry) => (
+ <ButtonBase
+ key={entry.id}
+
data-testid={`legend-${entry.id}`}
+ onClick={() =>
onLegendClick(String(entry.value))}
+ aria-label={String(entry.value)}
+ className="legend-button"
+ >
+ <Box
className="legend-color-box" style={{ backgroundColor: entry.color }} />
Review Comment:
Legend color box still uses inline style={{ backgroundColor: entry.color }}.
Since legend colors are dynamic (active vs inactive), this may be intentional —
but consider using a CSS custom property (style={{ '--legend-color':
entry.color }}) with a SCSS rule, or document why inline style is required here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
