pawarprasad123 commented on code in PR #737:
URL: https://github.com/apache/atlas/pull/737#discussion_r3860111439
##########
dashboardv2/public/css/scss/graph.scss:
##########
@@ -99,6 +99,13 @@
margin-bottom: 5px;
text-align: left;
+ &.entity-list-item {
Review Comment:
Ellipsis is applied to ul > li.entity-list-item, but buildListItem() in
RelationshipLayoutView.js (~lines 359–361) puts long text inside <a
class="entity-type-name">. Ellipsis on <li> does not reliably truncate nested
inline <a> text.
Consider moving truncation to .entity-list-item .entity-type-name (with
display: inline-block; max-width: 100%) — same pattern used for
.relationship-card-link in relationship.scss. Truncation on <li> alone may not
clip anchor content.
confirm this, and update
##########
dashboard/src/views/DetailPage/EntityDetailTabs/__tests__/RelationshipLineage.test.tsx:
##########
Review Comment:
This test name implies class verification but only checks SVG rendering.
Please assert text-deleted on the link and/or deleted-relation on the <li>
after opening the drawer with a DELETED entity.
##########
dashboard/src/views/DetailPage/EntityDetailTabs/__tests__/RelationshipLineage.test.tsx:
##########
@@ -1217,6 +1217,41 @@ describe('RelationshipLineage', () => {
});
});
+ describe('Tooltip Rendering', () => {
+ it('should render LightTooltip for relationship nodes in
drawer', async () => {
Review Comment:
Please add negative/edge cases: deleted status styling, missing typeName
label, and at least one assertion that tooltip title matches full untruncated
text. Classic UI tooltip lifecycle has zero automated coverage.
##########
dashboard/src/views/DetailPage/EntityDetailTabs/RelationshipLineage.tsx:
##########
@@ -48,28 +48,40 @@ import { CloseIcon, LightTooltip } from
"@components/muiComponents";
import { useAppSelector } from "@hooks/reducerHook";
import { Link as MUILink } from "@mui/material";
+interface CustomLinkProps {
+ href: string;
+ status: string;
+ guid: string;
Review Comment:
guid is declared in the interface and destructured but never used.
Remove unused guid from CustomLinkProps and destructuring, or use it (e.g.
as key / data-testid).
verify and update
##########
dashboardv2/public/js/views/graph/RelationshipLayoutView.js:
##########
@@ -271,27 +271,29 @@ define([
}
return entity;
}.bind(this),
- getdefault = function(options) {
- return "<pre class='entity-type-name' style='color:" +
options.color + "'>" + options.name + "</pre>";
+ getdefault = function (options) {
Review Comment:
Dead code
(~lines 274–336)
getdefault, getWithButton, getEntityTypelist, and getElement are defined but
getElement is never called. The list uses buildListItem() (~lines 352–387).
Refactored color-class logic in the unused path adds review noise without
effect.
getElement/getEntityTypelist appear unused in searchNode. Consider removing
dead code or clarifying if another code path uses it. Also, pre-existing return
entityTypeHtml + "</pre>"; (~line 328) looks like a bug.
##########
dashboard/src/views/DetailPage/EntityDetailTabs/RelationshipLineage.tsx:
##########
Review Comment:
If adding types, consider removing @ts-nocheck incrementally — otherwise the
type improvements provide limited value.
##########
dashboardv2/public/js/views/graph/RelationshipLayoutView.js:
##########
@@ -152,14 +152,14 @@ define([
}
return { nodes: nodes, links: links };
},
- onRender: function() {
+ onRender: function () {
Review Comment:
Large formatting-only diff
~160 lines are whitespace/function() → function () changes unrelated to the
fix. Makes review harder.
update this in the changed files
##########
dashboardv2/public/css/scss/theme.scss:
##########
@@ -536,7 +537,8 @@ hr[size="10"] {
}
.tooltip-inner {
- max-width: none;
+ max-width: 300px;
Review Comment:
Global Classic UI tooltip width change — possible regression
.tooltip-inner { max-width: 300px } is global. Other Classic UI tooltips
that relied on max-width: none may now wrap/truncate unexpectedly.
Scope tooltip max-width to relationship views (e.g.
.relationship-node-details .tooltip-inner) instead of changing the global
.tooltip-inner rule.
##########
dashboard/src/views/DetailPage/EntityDetailTabs/RelationshipLineage.tsx:
##########
@@ -48,28 +48,40 @@ import { CloseIcon, LightTooltip } from
"@components/muiComponents";
import { useAppSelector } from "@hooks/reducerHook";
import { Link as MUILink } from "@mui/material";
+interface CustomLinkProps {
+ href: string;
+ status: string;
+ guid: string;
+ name: string;
+ typeName: string;
+ params: URLSearchParams | string;
+}
+
const CustomLink = ({
href,
status,
- entityColor,
guid,
name,
typeName,
params
-}: any): any => {
+}: CustomLinkProps): JSX.Element => {
+ const displayLabel = typeName ? `${name} (${typeName})` : name;
return (
<li className={status}>
- <MUILink
- component={RouterLink}
- to={{
- pathname: href,
- search: params.toString() ? params.toString() : ""
- }}
- style={{ color: entityColor }}
- replace={true}
- >
- {name} ({typeName})
- </MUILink>
+ <LightTooltip title={displayLabel}>
Review Comment:
(~lines 71–84, 541–549)
MUI Tooltip expects a child that holds a ref. MUILink + RouterLink can
trigger console warnings. Wrapping in <span style={{ display: 'block',
overflow: 'hidden' }}> is safer.
##########
dashboardv2/public/css/scss/theme.scss:
##########
@@ -536,7 +537,8 @@ hr[size="10"] {
}
.tooltip-inner {
- max-width: none;
+ max-width: 300px;
+ word-wrap: break-word;
Review Comment:
Deprecated CSS property
word-wrap: break-word is legacy; prefer overflow-wrap: break-word.
--
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]
