pawarprasad123 commented on code in PR #728:
URL: https://github.com/apache/atlas/pull/728#discussion_r3819610304
##########
dashboard/src/views/DashboardOverview/LatestEntitiesList.tsx:
##########
@@ -146,54 +148,37 @@ const LatestEntitiesList = memo(({ entities, isLoading,
error }: LatestEntitiesL
if (isLoading) return null;
return (
- <Paper
- elevation={1}
- sx={{
- padding: 2,
- borderRadius: 2,
- minHeight: 340,
- minWidth: 0,
- width: "100%",
- flex: 1,
- boxSizing: "border-box",
- transition: "box-shadow 0.3s ease",
- "&:hover": { boxShadow: 4 }
- }}
- >
- <Box sx={{ pb: 2, borderBottom: "1px solid",
borderColor: "divider" }}>
+ <Paper elevation={1} className="latest-entities-paper">
+ <Box className="latest-entities-header">
<Stack direction="row"
justifyContent="space-between" alignItems="center">
- <Typography sx={{ fontSize: "1rem",
fontWeight: 600, color: "#1a1a1a" }}>
+ <Typography
className="latest-entities-title">
Latest Entities Created
</Typography>
<Link
component="button"
onClick={handleViewAll}
- sx={{
- fontSize: "0.875rem",
- cursor: "pointer",
- textDecoration: "none",
- color: "primary.main"
- }}
+
className="latest-entities-view-all"
aria-label="View all entities"
+ color="primary.main"
Review Comment:
MUI Link color expects "primary" | "secondary" | ..., not "primary.main".
This is invalid and may be ignored. Use color="primary" or style via SCSS.
##########
dashboard/src/components/muiComponents.tsx:
##########
@@ -68,6 +69,69 @@ const LightTooltip = styled(({ className, ...props }: any)
=> (
}
}));
+import { TooltipProps } from '@mui/material/Tooltip';
+import { SxProps, Theme } from '@mui/material/styles';
Review Comment:
line 72-73
These imports sit after the LightTooltip definition. They should be at the
top with the other imports. ESLint didn’t error here today, but this violates
standard import ordering and may fail stricter CI later.
move to the top ~line18
##########
dashboard/src/components/__tests__/muiComponents.test.tsx:
##########
@@ -69,6 +70,39 @@ describe('muiComponents', () => {
expect(screen.getByText('Tooltip Child')).toBeTruthy()
})
+ describe('OverflowTooltip', () => {
Review Comment:
OverflowTooltip “disables tooltip when not overflowed”, Admits JSDOM can’t
measure overflow; only checks render
“enables tooltip on resize”, Dispatches resize but never mocks scrollWidth >
clientWidth
“renders extremely long entity name without crashing”, Doesn’t assert
ellipsis/truncation CSS or layout classes
“slices to exactly 7 items”, Duplicates existing “renders link when guid
present and slices to seven”
“uses top-level __timestamp”, Assertion weakened — no longer checks
timestamp text
##########
dashboard/src/views/DashboardOverview/LatestEntitiesList.scss:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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.
+ */
+
+.latest-entities-paper {
+ padding: 16px;
+ border-radius: 8px;
+ min-height: 340px;
+ min-width: 0;
+ width: 100%;
+ flex: 1;
+ box-sizing: border-box;
+ transition: box-shadow 0.3s ease;
+
+ &:hover {
+ box-shadow: 0px 2px 4px -1px rgba(0,0,0,0.2), 0px 4px 5px 0px
rgba(0,0,0,0.14), 0px 1px 10px 0px rgba(0,0,0,0.12);
+ }
+}
+
+.latest-entities-header {
+ padding-bottom: 16px;
+ border-bottom: 1px solid rgba(0, 0, 0, 0.12);
+}
+
+.latest-entities-title {
+ font-size: 1rem;
+ font-weight: 600;
+ color: #1a1a1a;
Review Comment:
~lines 39–42, 88–91
Rest of dashboard often uses MUI theme tokens. Hardcoded hex values may not
match dark theme or future theme changes. Minor, but worth aligning with theme
variables if available.
##########
dashboard/src/components/muiComponents.tsx:
##########
@@ -68,6 +69,69 @@ const LightTooltip = styled(({ className, ...props }: any)
=> (
}
}));
+import { TooltipProps } from '@mui/material/Tooltip';
+import { SxProps, Theme } from '@mui/material/styles';
+
+interface OverflowTooltipProps extends Omit<TooltipProps, 'children'> {
+ children: React.ReactElement;
+ wrapperComponent?: React.ElementType;
+ wrapperSx?: SxProps<Theme>;
+}
+
+const OverflowTooltip = ({ title, children, wrapperComponent, wrapperSx,
...props }: OverflowTooltipProps) => {
+ const textElementRef = React.useRef<HTMLElement>(null);
+ const [isOverflowed, setIsOverflowed] = React.useState(false);
+
+ const checkOverflow = () => {
+ if (textElementRef.current) {
+ setIsOverflowed(
+ textElementRef.current.scrollWidth > textElementRef.current.clientWidth
+ );
+ }
+ };
+
+ React.useEffect(() => {
+ checkOverflow();
+ window.addEventListener("resize", checkOverflow);
+ return () => {
+ window.removeEventListener("resize", checkOverflow);
+ };
+ }, [children, title]);
Review Comment:
children is a new reference most renders → effect re-runs and re-attaches
the resize listener often. Prefer depending on title only, or measure in a
layout effect without children in deps.
##########
dashboard/src/views/DashboardOverview/LatestEntitiesList.tsx:
##########
@@ -207,55 +192,40 @@ const LatestEntitiesList = memo(({ entities, isLoading,
error }: LatestEntitiesL
<ListItem
key={entityGuid
|| displayName}
disablePadding
- sx={{
- py: 1,
-
borderBottom: "1px solid",
-
borderColor: "divider",
-
"&:last-child": { borderBottom: "none" }
- }}
+
className="latest-entities-list-item"
>
<Stack
width="100%" direction="row" justifyContent="space-between" alignItems="center">
- <Stack
direction="row" spacing={0.5} alignItems="center" flexWrap="wrap" flex={1}
minWidth={0} mr={1}>
+ <Stack
direction="row" spacing={0.5} alignItems="center" flexWrap="nowrap" flex={1}
minWidth={0} mr={1}>
{detailHref ? (
-
<Link
-
component={RouterLink}
-
to={detailHref}
-
underline="hover"
-
color="primary"
-
sx={{
-
fontSize: "0.875rem",
-
overflow: "hidden",
-
textOverflow: "ellipsis",
-
cursor: "pointer",
-
maxWidth: "100%"
-
}}
-
>
-
{displayName}
-
</Link>
+
<OverflowTooltip title={displayName} arrow placement="top">
+
<Link
+
component={RouterLink}
+
to={detailHref}
+
underline="hover"
+
color="primary"
+
className="latest-entities-entity-name
latest-entities-entity-name-link"
+
>
+
{displayName}
+
</Link>
+
</OverflowTooltip>
) : (
-
<Typography
-
component="span"
-
sx={{
-
fontSize: "0.875rem",
-
fontWeight: 500,
-
color: "text.primary"
-
}}
-
>
-
{displayName}
-
</Typography>
+
<OverflowTooltip title={displayName} arrow placement="top">
+
<Typography
+
component="span"
+
className="latest-entities-entity-name
latest-entities-entity-name-fallback"
+
>
+
{displayName}
+
</Typography>
+
</OverflowTooltip>
)}
<Typography
component="span"
-
sx={{
-
fontSize: "0.875rem",
-
color: "#6c757d",
-
flexShrink: 0
-
}}
+
className="latest-entities-type-name"
Review Comment:
line 221-226
Only displayName is wrapped in OverflowTooltip. A long (typeName) could
still overflow because .latest-entities-type-name has flex-shrink: 0. Consider
truncating type names too, or allow them to shrink.
verify this.
##########
dashboard/src/components/muiComponents.tsx:
##########
@@ -68,6 +69,69 @@ const LightTooltip = styled(({ className, ...props }: any)
=> (
}
}));
+import { TooltipProps } from '@mui/material/Tooltip';
+import { SxProps, Theme } from '@mui/material/styles';
+
+interface OverflowTooltipProps extends Omit<TooltipProps, 'children'> {
+ children: React.ReactElement;
+ wrapperComponent?: React.ElementType;
+ wrapperSx?: SxProps<Theme>;
+}
+
+const OverflowTooltip = ({ title, children, wrapperComponent, wrapperSx,
...props }: OverflowTooltipProps) => {
+ const textElementRef = React.useRef<HTMLElement>(null);
+ const [isOverflowed, setIsOverflowed] = React.useState(false);
+
+ const checkOverflow = () => {
+ if (textElementRef.current) {
+ setIsOverflowed(
+ textElementRef.current.scrollWidth > textElementRef.current.clientWidth
+ );
+ }
+ };
+
+ React.useEffect(() => {
+ checkOverflow();
+ window.addEventListener("resize", checkOverflow);
+ return () => {
+ window.removeEventListener("resize", checkOverflow);
+ };
+ }, [children, title]);
+
+ const child = wrapperComponent || wrapperSx ? (
+ <Box
+ component={wrapperComponent || 'span'}
+ ref={textElementRef}
+ sx={{
+ display: "inline-flex",
+ minWidth: 0,
+ width: "100%",
+ alignItems: "center",
+ overflow: "hidden",
+ textOverflow: "ellipsis",
+ whiteSpace: "nowrap",
+ ...wrapperSx
+ }}
+ >
+ {children}
+ </Box>
+ ) : (
+ React.cloneElement(children, { ref: textElementRef })
Review Comment:
line 101–119
RouterLink + MUI Link may not always merge refs cleanly with cloneElement.
Safer options:
Use the existing wrapperSx path and pass wrapperComponent="span" from
LatestEntitiesList, or
Wrap children in a truncating Box with the ref (same pattern as the
wrapperComponent branch).
--
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]
