pawarprasad123 commented on code in PR #728:
URL: https://github.com/apache/atlas/pull/728#discussion_r3961115380
##########
dashboard/src/components/__tests__/muiComponents.test.tsx:
##########
@@ -69,6 +70,100 @@ describe('muiComponents', () => {
expect(screen.getByText('Tooltip Child')).toBeTruthy()
})
+ describe('OverflowTooltip', () => {
+ let triggerResize: ResizeObserverCallback | undefined
+ const originalResizeObserver = global.ResizeObserver
+
+ beforeAll(() => {
+ global.ResizeObserver = class {
+ constructor(callback: ResizeObserverCallback) {
+ triggerResize = callback
+ }
+ observe = jest.fn()
+ unobserve = jest.fn()
+ disconnect = jest.fn()
+ } as any
Review Comment:
as any on ResizeObserver mock — acceptable in tests, but slightly weakens
the type-safety goal of this PR
##########
dashboard/src/views/DashboardOverview/__tests__/LatestEntitiesList.test.tsx:
##########
@@ -624,4 +622,68 @@ describe('LatestEntitiesList', () => {
)
expect(screen.getByText('Created today')).toBeInTheDocument()
})
+
+ it('renders fallback Typography when detailHref is absent (no guid)',
() => {
+ render(
+ <MemoryRouter>
+ <LatestEntitiesList
+ entities={[
+ {
+ name:
'EntityWithoutGuid',
+ typeName: 'T',
+ attributes: {
__timestamp: Date.now() },
+ },
+ ]}
+ />
+ </MemoryRouter>,
+ )
+ const fallbackText = screen.getByText('EntityWithoutGuid')
+ expect(fallbackText).toBeInTheDocument()
+ expect(fallbackText.tagName).toBe('SPAN')
+
expect(fallbackText).toHaveClass('latest-entities-entity-name-fallback')
+ })
+
+
+ it('renders extremely long entity name without crashing', () => {
+ const longName = 'A'.repeat(500)
+ render(
+ <MemoryRouter>
+ <LatestEntitiesList
+ entities={[
+ {
+ guid: 'g1',
+ name: longName,
+ typeName: 'T',
+ attributes: {
__timestamp: Date.now() },
+ },
+ ]}
+ />
+ </MemoryRouter>,
+ )
+ const link = screen.getByRole('link', { name: longName })
+ expect(link).toBeInTheDocument()
+ expect(link.textContent).toBe(longName)
+
+ const span = link.parentElement
+ expect(span).toHaveStyle('overflow: hidden')
+ expect(span).toHaveStyle('text-overflow: ellipsis')
+ expect(span).toHaveStyle('white-space: nowrap')
+ })
+
Review Comment:
Please add a test for extremely long typeName (e.g. 'B'.repeat(300))
asserting the row renders without layout break and truncation styles are
applied on the type wrapper.
##########
dashboard/src/views/DashboardOverview/__tests__/LatestEntitiesList.test.tsx:
##########
@@ -624,4 +622,89 @@ describe('LatestEntitiesList', () => {
)
expect(screen.getByText('Created today')).toBeInTheDocument()
})
+
+ it('renders fallback Typography when detailHref is absent (no guid)',
() => {
Review Comment:
line 159–169 vs 626–644
Two overlapping tests for no-guid fallback — could be consolidated
##########
dashboard/src/views/DashboardOverview/LatestEntitiesList.scss:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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 {
Review Comment:
Uses 2-space indent; AuditResults.scss uses 4-space — inconsistent with some
existing SCSS
##########
dashboard/src/views/DashboardOverview/LatestEntitiesList.scss:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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: rgba(0, 0, 0, 0.87);
+}
+
+.latest-entities-view-all {
+ font-size: 0.875rem;
+ cursor: pointer;
+ text-decoration: none;
+}
+
+.latest-entities-empty {
+ padding-top: 16px;
+}
+
+.latest-entities-list {
+ padding-top: 16px;
+}
+
+.latest-entities-list-item {
+ padding-top: 8px;
+ padding-bottom: 8px;
+ border-bottom: 1px solid rgba(0, 0, 0, 0.12);
+
+ &:last-child {
+ border-bottom: none;
+ }
+}
+
+.latest-entities-entity-name {
+ font-size: 0.875rem;
+}
+
+.latest-entities-entity-name-link {
Review Comment:
.latest-entities-type-name has ellipsis rules, but
.latest-entities-entity-name / .latest-entities-entity-name-link do not.
Truncation currently depends on OverflowTooltip's inline sx. For visual
consistency, add the same truncation rules to the entity name classes (or move
all truncation into SCSS and drop inline sx).
##########
dashboard/src/components/muiComponents.tsx:
##########
@@ -68,58 +71,97 @@ const LightTooltip = styled(({ className, ...props }: any)
=> (
}
}));
-interface ButtonProps {
- children?: any;
- variant?: string;
- color: string;
- onClick: any;
- sx?: any;
- size?: string;
- endIcon?: any;
- startIcon?: any;
- className?: string;
- disabled?: boolean;
+
+interface OverflowTooltipProps extends Omit<TooltipProps, "children"> {
+ children: React.ReactElement;
+ wrapperSx?: SxProps<Theme>;
+ wrapperClassName?: string;
}
+const OverflowTooltip = ({ title, children, wrapperSx, wrapperClassName,
...props }: OverflowTooltipProps) => {
+ const textElementRef = React.useRef<HTMLElement>(null);
+ const [isOverflowed, setIsOverflowed] = React.useState(false);
+
+ const checkOverflow = React.useCallback(() => {
+ if (textElementRef.current) {
+ const el = textElementRef.current;
+ setIsOverflowed(
+ el.scrollWidth > el.clientWidth ||
+ el.scrollWidth > el.getBoundingClientRect().width
Review Comment:
el.scrollWidth > el.clientWidth ||
el.scrollWidth > el.getBoundingClientRect().width
The two conditions overlap in most cases. The subpixel case is covered by
the getBoundingClientRect() check — consider a short comment explaining why
both are needed, or simplify if redundant.
Scope creep (non-blocking): CustomButton / LightTooltip refactors touch 6
unrelated files. Necessary for type safety, but increases regression risk
beyond the widget fix.
##########
dashboard/src/views/DashboardOverview/__tests__/LatestEntitiesList.test.tsx:
##########
@@ -624,4 +622,89 @@ describe('LatestEntitiesList', () => {
)
expect(screen.getByText('Created today')).toBeInTheDocument()
})
+
+ it('renders fallback Typography when detailHref is absent (no guid)',
() => {
+ render(
+ <MemoryRouter>
+ <LatestEntitiesList
+ entities={[
+ {
+ name:
'EntityWithoutGuid',
+ typeName: 'T',
+ attributes: {
__timestamp: Date.now() },
+ },
+ ]}
+ />
+ </MemoryRouter>,
+ )
+ const fallbackText = screen.getByText('EntityWithoutGuid')
+ expect(fallbackText).toBeInTheDocument()
+ expect(fallbackText.tagName).toBe('SPAN')
+
expect(fallbackText).toHaveClass('latest-entities-entity-name-fallback')
+ })
+
+
+ it('renders extremely long entity name without crashing', () => {
+ const longName = 'A'.repeat(500)
+ render(
+ <MemoryRouter>
+ <LatestEntitiesList
+ entities={[
+ {
+ guid: 'g1',
+ name: longName,
+ typeName: 'T',
+ attributes: {
__timestamp: Date.now() },
+ },
+ ]}
+ />
+ </MemoryRouter>,
+ )
+ const link = screen.getByRole('link', { name: longName })
+ expect(link).toBeInTheDocument()
+ expect(link.textContent).toBe(longName)
+
+ const span = link.parentElement
+ expect(span).toHaveStyle('overflow: hidden')
+ expect(span).toHaveStyle('text-overflow: ellipsis')
+ expect(span).toHaveStyle('white-space: nowrap')
+ })
+
+ it('renders gracefully when typeName is missing', () => {
+ render(
+ <MemoryRouter>
+ <LatestEntitiesList
+ entities={[
+ {
+ guid: 'g1',
+ name: 'NamelessType',
+ attributes: {
__timestamp: Date.now() },
+ },
+ ]}
+ />
+ </MemoryRouter>,
+ )
+ expect(screen.getByText('(Entity)')).toBeInTheDocument()
+ })
+
+ it('renders extremely long typeName without breaking layout', () => {
+ const longTypeName = 'B'.repeat(300)
+ render(
+ <MemoryRouter>
+ <LatestEntitiesList
+ entities={[
+ {
+ guid: 'g1',
+ name:
'EntityWithLongType',
+ typeName: longTypeName,
+ attributes: {
__timestamp: Date.now() },
+ },
+ ]}
+ />
+ </MemoryRouter>,
+ )
+ const typeElement = screen.getByText(`(${longTypeName})`)
+ expect(typeElement).toBeInTheDocument()
+
expect(typeElement.parentElement).toHaveClass('latest-entities-type-wrapper')
+ })
Review Comment:
Gaps:
Long name test checks wrapper overflow styles but does not assert tooltip on
hover
No test for both long entity name and long typeName together (the actual bug
scenario)
No assertion that .latest-entities-name-wrapper class is applied
No layout assertion for max-width: 45% on type wrapper
Duplicate coverage: renders span without link when guid missing (L159) vs
renders fallback Typography when detailHref is absent (L626)
##########
dashboard/src/components/muiComponents.tsx:
##########
@@ -68,58 +71,97 @@ const LightTooltip = styled(({ className, ...props }: any)
=> (
}
}));
-interface ButtonProps {
- children?: any;
- variant?: string;
- color: string;
- onClick: any;
- sx?: any;
- size?: string;
- endIcon?: any;
- startIcon?: any;
- className?: string;
- disabled?: boolean;
+
+interface OverflowTooltipProps extends Omit<TooltipProps, "children"> {
+ children: React.ReactElement;
+ wrapperSx?: SxProps<Theme>;
+ wrapperClassName?: string;
}
+const OverflowTooltip = ({ title, children, wrapperSx, wrapperClassName,
...props }: OverflowTooltipProps) => {
+ const textElementRef = React.useRef<HTMLElement>(null);
+ const [isOverflowed, setIsOverflowed] = React.useState(false);
+
+ const checkOverflow = React.useCallback(() => {
+ if (textElementRef.current) {
+ const el = textElementRef.current;
+ setIsOverflowed(
+ el.scrollWidth > el.clientWidth ||
+ el.scrollWidth > el.getBoundingClientRect().width
+ );
+ }
+ }, []);
+
+ React.useEffect(() => {
+ checkOverflow();
+ const element = textElementRef.current;
+ if (element) {
+ // One ResizeObserver per instance — acceptable for small lists (e.g.
dashboard widgets).
+ // If this component is used in large virtualized lists, consider
lifting a shared
+ // ResizeObserver to a context provider to reduce observer count.
+ const resizeObserver = new ResizeObserver(() => checkOverflow());
+ resizeObserver.observe(element);
+ return () => resizeObserver.disconnect();
+ }
+ }, [title, checkOverflow]);
+
+ const child = (
+ <Box
Review Comment:
SCSS sets display: block on wrappers, but sx sets display: inline-flex. MUI
sx wins, which partially undermines the SCSS migration goal. Consider moving
truncation styles fully into SCSS and keeping OverflowTooltip generic.
##########
dashboard/src/components/__tests__/muiComponents.test.tsx:
##########
@@ -21,11 +21,12 @@
*/
import React from 'react'
-import { render, screen, fireEvent } from '@testing-library/react'
+import { render, screen, fireEvent, act } from '@testing-library/react'
Review Comment:
Gaps:
No test for ResizeObserver.disconnect() on unmount
No test that wrapperClassName is applied to the wrapper
No test when title prop changes and overflow state should update
##########
dashboard/src/views/DashboardOverview/LatestEntitiesList.scss:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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: rgba(0, 0, 0, 0.87);
+}
+
+.latest-entities-view-all {
+ font-size: 0.875rem;
+ cursor: pointer;
+ text-decoration: none;
+}
+
+.latest-entities-empty {
+ padding-top: 16px;
+}
+
+.latest-entities-list {
+ padding-top: 16px;
+}
+
+.latest-entities-list-item {
+ padding-top: 8px;
+ padding-bottom: 8px;
+ border-bottom: 1px solid rgba(0, 0, 0, 0.12);
+
+ &:last-child {
+ border-bottom: none;
+ }
+}
+
+.latest-entities-entity-name {
+ font-size: 0.875rem;
+}
+
+.latest-entities-entity-name-link {
+ cursor: pointer;
+}
+
+.latest-entities-entity-name-fallback {
+ font-weight: 500;
+ color: rgba(0, 0, 0, 0.87);
+}
+
+.latest-entities-type-name {
+ font-size: 0.875rem;
+ color: rgba(0, 0, 0, 0.6);
+ overflow: hidden;
+ text-overflow: ellipsis;
+ white-space: nowrap;
+ min-width: 0;
+}
+
+.latest-entities-timestamp {
+ font-size: 0.8125rem;
+ color: rgba(0, 0, 0, 0.6);
+ flex-shrink: 0;
+ margin-left: 8px;
+ white-space: nowrap;
+}
+
+.latest-entities-name-wrapper {
+ display: block;
+ /* Allow this column to grow and shrink; min-width: 0 enables text
truncation inside flex */
+ flex: 1 1 auto;
+ min-width: 0;
+ width: auto;
+}
+
+.latest-entities-type-wrapper {
+ display: block;
+ flex: 0 1 auto;
+ flex-shrink: 0;
Review Comment:
flex: 0 1 auto allows shrinking, but flex-shrink: 0 overrides it. Pick one
intent and remove the redundant rule.
--
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]
