pawarprasad123 commented on code in PR #703:
URL: https://github.com/apache/atlas/pull/703#discussion_r3859890022
##########
dashboard/src/views/Statistics/EntityStatsChart.tsx:
##########
@@ -87,21 +98,24 @@ const EntityStatsChart = ({
cursor={{ stroke: "rgba(0, 0, 0, 0.1)",
strokeWidth: 2 }}
/>
<Legend
- onClick={(e) => {
- if (e && e.id) {
-
onLegendClick(String(e.id));
- }
- }}
-
payload={Object.keys(activeKeys).map((key) => ({
- id: key,
- type: "square",
- value: key,
- color:
- activeKeys[key as keyof
ActiveKeys] === true
- ?
getColorForKey(key)
- : "#d3d3d3",
- inactive: !activeKeys[key as
keyof ActiveKeys],
- }))}
+ content={() => (
Review Comment:
Consider adding a focused EntityStatsChart.test.tsx for the custom legend
renderer (click toggles, inactive styling, aria-label). Integration tests in
EntityStats cover some paths but not this component in isolation.
##########
dashboard/src/styles/stats.scss:
##########
@@ -60,3 +60,191 @@
.classification-name-cell {
max-width: 400px;
}
+
+.chart-cursor-pointer {
+ cursor: pointer;
+}
+
+.chart-cursor-default {
+ cursor: default;
+}
+
+.chart-label-list {
+ font-size: 12px;
+ font-weight: 500;
+ fill: #1976d2;
+}
+
+.legend-button {
+ display: flex;
+ flex-direction: row;
+ align-items: center;
+ gap: 4px;
+ background: none;
+ padding: 0;
+ margin: 0;
+ border-radius: 4px;
+}
+
+.legend-color-box {
+ width: 12px;
+ height: 12px;
+ border-radius: 2px;
+}
+
+.legend-typography {
+ font-size: 0.875rem;
+}
+
+.legend-inactive {
+ color: #d3d3d3;
+}
+
+.legend-active {
+ color: #333;
+}
+
+.chart-card {
Review Comment:
Lines 107–131 (.chart-card, .chart-card-title)
New SCSS uses hardcoded values (#1976d2, #fff, #333) instead of MUI theme
tokens. This can break dark mode and diverge from primary.main, divider, etc.
that were removed from sx.
##########
dashboard/src/views/DashboardOverview/__tests__/EntityStatusDonut.test.tsx:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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 { render, screen, fireEvent } from "@testing-library/react";
Review Comment:
missing test:
1) Left-side status button clicks (aria-label="View Active entities"):
Separate code path from pie slice clicks
2) isLoading={true} returns null : Guard exists at line 69 but untested
3) All-zero totals (empty chartData): Edge case for pie rendering
4) Negative: invalid pie click payload: Defensive coverage
Also replace any in test mocks with typed interfaces to match the strict
typing goal of this PR.
##########
dashboard/src/views/Statistics/EntityStatsChart.tsx:
##########
@@ -87,21 +98,24 @@ const EntityStatsChart = ({
cursor={{ stroke: "rgba(0, 0, 0, 0.1)",
strokeWidth: 2 }}
/>
<Legend
- onClick={(e) => {
- if (e && e.id) {
-
onLegendClick(String(e.id));
- }
- }}
-
payload={Object.keys(activeKeys).map((key) => ({
- id: key,
- type: "square",
- value: key,
- color:
- activeKeys[key as keyof
ActiveKeys] === true
- ?
getColorForKey(key)
- : "#d3d3d3",
- inactive: !activeKeys[key as
keyof ActiveKeys],
- }))}
+ content={() => (
+ <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 }} />
+
<Typography variant="body2" className={`legend-typography ${entry.inactive ?
'legend-inactive' : 'legend-active'}`}>
Review Comment:
line 112 uses single quotes inside template literal while the file uses
double quotes elsewhere: Use double quotes for consistency.
##########
dashboard/src/views/Statistics/__tests__/EntityStats.test.tsx:
##########
@@ -165,15 +165,20 @@ jest.mock('recharts', () => ({
YAxis: ({ domain, tickFormatter }: any) => <div data-testid="y-axis"
data-domain={JSON.stringify(domain)} data-tick-formatter={tickFormatter ?
tickFormatter(10) : ''} />,
Review Comment:
line 41
till uses inline style object
PR description says inline styles were purged; this one remains. Either move
to SCSS (e.g. .chart-label-list-sm) or document why it stays inline.
##########
dashboard/src/styles/stats.scss:
##########
@@ -60,3 +60,191 @@
.classification-name-cell {
max-width: 400px;
}
+
+.chart-cursor-pointer {
+ cursor: pointer;
+}
+
+.chart-cursor-default {
+ cursor: default;
+}
+
+.chart-label-list {
+ font-size: 12px;
+ font-weight: 500;
+ fill: #1976d2;
+}
+
+.legend-button {
+ display: flex;
+ flex-direction: row;
+ align-items: center;
+ gap: 4px;
+ background: none;
+ padding: 0;
+ margin: 0;
+ border-radius: 4px;
+}
+
+.legend-color-box {
+ width: 12px;
+ height: 12px;
+ border-radius: 2px;
+}
+
+.legend-typography {
+ font-size: 0.875rem;
+}
+
+.legend-inactive {
+ color: #d3d3d3;
+}
+
+.legend-active {
+ color: #333;
+}
+
+.chart-card {
+ padding: 16px;
+ border-radius: 8px;
+ min-height: 200px;
+ transition: box-shadow 0.3s ease;
+}
+
+.chart-card:hover {
+ box-shadow: 0px 4px 6px -1px rgba(0,0,0,0.1), 0px 2px 4px -1px
rgba(0,0,0,0.06);
+}
+
+.chart-card-min-340 {
Review Comment:
this class is defined but never referenced. Either use it or remove it.
##########
dashboard/src/views/DashboardOverview/ClassificationDistributionCard.tsx:
##########
@@ -60,8 +66,13 @@ const ClassificationDistributionCard = memo(({ tag,
isLoading }: ClassificationD
);
const handleBarClick = useCallback(
- (entry: { name: string }) => {
- navigateToClassificationSearch(navigate, entry.name);
+ (barProps: unknown) => {
Review Comment:
no bar-click tests (positive or negative)
handleBarClick was refactored with a type guard, but tests only cover Y-axis
label truncation. Unlike EntityTypeBarChart.test.tsx (which has 'ignores bar
click when payload missing'), there are no bar-click tests here.
-
ClassificationDistributionCard.test.tsx line 78
Please add tests for handleBarClick: (1) valid payload navigates to
classification search, (2) invalid/null/non-object payload does not navigate.
EntityTypeBarChart already has the negative case — mirror that pattern here.
##########
dashboard/src/views/DashboardOverview/EntityStatusDonut.tsx:
##########
@@ -147,18 +117,15 @@ const EntityStatusDonut = memo(({ entity, isLoading }:
EntityStatusDonutProps) =
isAnimationActive
animationDuration={800}
animationEasing="ease-out"
-
activeIndex={activeIndex}
activeShape={renderActiveShape}
Review Comment:
pie hover highlight removed
Removed:
activeIndex state
onMouseEnter / onMouseLeave
activeIndex prop on <Pie>
activeShape={renderActiveShape} remains but may no longer activate on hover
in Recharts v3 without activeIndex.
please restore it using the v3 API
##########
dashboard/src/views/Statistics/EntityStatsChart.tsx:
##########
@@ -87,21 +98,24 @@ const EntityStatsChart = ({
cursor={{ stroke: "rgba(0, 0, 0, 0.1)",
strokeWidth: 2 }}
/>
<Legend
- onClick={(e) => {
- if (e && e.id) {
-
onLegendClick(String(e.id));
- }
- }}
-
payload={Object.keys(activeKeys).map((key) => ({
- id: key,
- type: "square",
- value: key,
- color:
- activeKeys[key as keyof
ActiveKeys] === true
- ?
getColorForKey(key)
- : "#d3d3d3",
- inactive: !activeKeys[key as
keyof ActiveKeys],
- }))}
+ content={() => (
+ <Stack direction="row"
spacing={2} justifyContent="center" mt={1}>
Review Comment:
Extract to a memoized component or useCallback to avoid recreating the
legend on each render (minor perf).
##########
dashboard/src/views/DashboardOverview/dashboardChartPalette.ts:
##########
@@ -25,6 +25,8 @@ export const ENTITY_STATUS_DONUT_COLORS = {
/** Active primary series / bar fill (aligned with Classification Distribution
bars) */
export const CHART_BAR_ACTIVE_BLUE = "#1976d2";
+
Review Comment:
Remove the double blank line.
--
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]
