Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces a new reusable Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/components/TruncatedText.test.tsx(1 hunks)frontend/src/components/ItemCardList.tsx(2 hunks)frontend/src/components/TruncatedText.tsx(1 hunks)frontend/src/pages/Home.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (9)
frontend/src/components/ItemCardList.tsx (2)
4-4: LGTM: Added import for the new TruncatedText component.The import statement is correctly added to use the new component.
43-43: LGTM: Implemented text truncation for item titles.Good implementation of the TruncatedText component with a reasonable maxLength of 40 characters. This will improve UI consistency by truncating potentially long item titles while displaying the full title in a tooltip on hover.
frontend/src/components/TruncatedText.tsx (1)
1-27: LGTM: Well-structured TruncatedText component.The component is well-designed with appropriate props and implementation:
- Takes required text and optional maxLength, className, and disabledTooltip props
- Handles truncation logic cleanly
- Uses a Tooltip for displaying the full text on hover
- Properly manages conditional rendering based on text length
The implementation is clean, simple, and follows React best practices.
frontend/src/pages/Home.tsx (4)
32-32: LGTM: Added import for the new TruncatedText component.The import statement is correctly added.
148-150: LGTM: Implemented text truncation for event names.Good implementation of the TruncatedText component with a reasonable maxLength of 40 characters.
185-185: LGTM: Implemented text truncation for chapter names.Good implementation of the TruncatedText component with a reasonable maxLength of 40 characters.
297-297: LGTM: Implemented text truncation for post titles.Good implementation of the TruncatedText component with a reasonable maxLength of 40 characters.
frontend/__tests__/unit/components/TruncatedText.test.tsx (2)
1-22: LGTM: Comprehensive test coverage for TruncatedText component.The test suite covers the main functionality of the component:
- Rendering full text when shorter than maxLength
- Truncating long text with ellipsis
- Displaying tooltip on hover
21-21: Verify tooltip implementation in tests.The test is checking for a
titleattribute, but the component is using a Tooltip component rather than setting a title attribute directly. This test might be passing incorrectly or relying on implementation details.Consider using a tooltip testing library or verifying if the Tooltip component actually adds a title attribute to its children. If needed, modify the test to match the actual implementation.
frontend/src/pages/Home.tsx
Outdated
| <h3 className="truncate text-wrap md:text-nowrap"> | ||
| <TruncatedText text={project.name} maxLength={40} /> | ||
| </h3> |
There was a problem hiding this comment.
Remove redundant heading element.
There's a redundant nesting of h3 elements. The parent element is already an h3, and another h3 is nested inside the anchor tag.
Apply this fix:
-<h3 className="mb-2 text-lg font-semibold">
<a href={`/projects/${project.key}`} className="hover:underline">
- <h3 className="truncate text-wrap md:text-nowrap">
<TruncatedText text={project.name} maxLength={40} />
- </h3>
</a>
-</h3>
+<h3 className="mb-2 text-lg font-semibold">
+ <a href={`/projects/${project.key}`} className="hover:underline">
+ <span className="truncate text-wrap md:text-nowrap">
+ <TruncatedText text={project.name} maxLength={40} />
+ </span>
+ </a>
+</h3>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h3 className="truncate text-wrap md:text-nowrap"> | |
| <TruncatedText text={project.name} maxLength={40} /> | |
| </h3> | |
| <h3 className="mb-2 text-lg font-semibold"> | |
| <a href={`/projects/${project.key}`} className="hover:underline"> | |
| <span className="truncate text-wrap md:text-nowrap"> | |
| <TruncatedText text={project.name} maxLength={40} /> | |
| </span> | |
| </a> | |
| </h3> |
yashgoyal0110
force-pushed
the
feat/TruncatedText-component
branch
2 times, most recently
from
269ff95 to
7598ca8
Compare
yashgoyal0110
force-pushed
the
feat/TruncatedText-component
branch
from
f5c0fac to
1c1dd11
Compare
yashgoyal0110
force-pushed
the
feat/TruncatedText-component
branch
from
1c1dd11 to
269ff95
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/TruncatedText.tsx (3)
3-11: Prop handling could be improved to support nullable text values.The component correctly defines the props and uses useCallback for the truncation check function. However, it doesn't handle cases where
textmight be undefined or null, which could lead to issues.Consider adding a fallback for text to handle undefined or null values:
-export const TruncatedText = ({ text, className = '' }: { text: string; className?: string }) => { +export const TruncatedText = ({ text = '', className = '' }: { text?: string; className?: string }) => { const textRef = useRef<HTMLSpanElement>(null) const checkTruncation = useCallback(() => { const element = textRef.current if (element) { - element.title = text + element.title = text || '' } }, [text])
13-27: Effect setup is appropriate but consider optimizing resize handlers.The useEffect hook correctly sets up and cleans up event listeners and the ResizeObserver. However, both the ResizeObserver and the window resize event listener trigger the same function, which might be redundant in some cases.
Since ResizeObserver already captures element size changes, you might only need the window resize listener for specific cases where the ResizeObserver might not trigger. Consider if both are necessary for your use case, or if you can rely primarily on the ResizeObserver.
1-37: Consider adding a check for actual text truncation.The component currently always sets the title attribute regardless of whether the text is actually truncated. For better performance and user experience, consider adding a check to determine if the text is actually truncated before setting the title.
You could modify the checkTruncation function to check if the text is actually truncated:
const checkTruncation = useCallback(() => { const element = textRef.current if (element) { - element.title = text + // Only set title if text is truncated + if (element.scrollWidth > element.clientWidth) { + element.title = text + } else { + element.removeAttribute('title') + } } }, [text])However, based on the previous comments, it was agreed that "It's fine to always have the title set to text," so this suggestion is optional.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/components/TruncatedText.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/RepositoriesCard.tsx (1)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-37)
🔇 Additional comments (3)
frontend/src/components/RepositoriesCard.tsx (2)
16-16: Added import for TruncatedText component appropriately.The import has been correctly added for the TruncatedText component that will be used to handle text truncation.
63-63: Good implementation of TruncatedText for repository names.Replacing the direct text rendering with the TruncatedText component is a good approach. This will handle repository names that are too long by truncating them with an ellipsis and showing the full name on hover via the title attribute, improving the user experience.
frontend/src/components/TruncatedText.tsx (1)
29-36: CSS classes for truncation are well-implemented.The span element correctly uses appropriate CSS classes for text truncation (
overflow-hidden,truncate,text-ellipsis,whitespace-nowrap). The component also accepts an optional className prop to allow for additional styling, which provides good flexibility.
|
I have fixed test-cases |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/TruncatedText.test.tsx (2)
15-20: Consider validating the className prop.While your test correctly checks the text content and title attribute, it doesn't explicitly verify that the className prop is properly passed to the component. Since your test provides "w-auto" as className, consider adding an assertion to check it's applied.
test('renders full text when it fits within the container', () => { render(<TruncatedText text="Short text" className="w-auto" />) const textElement = screen.getByText('Short text') expect(textElement).toBeInTheDocument() expect(textElement).toHaveAttribute('title', 'Short text') + expect(textElement).toHaveClass('w-auto') })
12-27: Add tests for ResizeObserver functionality and edge cases.Your test coverage could be improved by adding tests for:
- Verification that the ResizeObserver is created and cleaned up properly
- Testing behavior with empty strings or very short text
- Testing how the component handles window resize events
Example of testing cleanup:
test('cleans up observer and event listener on unmount', () => { const { unmount } = render(<TruncatedText text={longText} />) // Mock the cleanup functions const disconnectSpy = jest.spyOn(global.ResizeObserver.prototype, 'disconnect') const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener') // Unmount the component unmount() // Verify cleanup was called expect(disconnectSpy).toHaveBeenCalled() expect(removeEventListenerSpy).toHaveBeenCalledWith('resize', expect.any(Function)) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/TruncatedText.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/__tests__/unit/components/TruncatedText.test.tsx (1)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-37)
🔇 Additional comments (1)
frontend/__tests__/unit/components/TruncatedText.test.tsx (1)
4-10: Good implementation of the ResizeObserver mock.You've correctly implemented a mock for ResizeObserver which is essential since it's not available in the JSDOM testing environment. The mock provides all the necessary methods that the component will call.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
You keep ignoring my requests to work according to the recommended guidelines. After this PR in order to avoid resource/time wasting I'm going to start approving the tests runs once per day only.
|
|
* commit * Update pnpm-lock.yaml * Update vite.config.ts * Update package.json * truncate text component * commit * commit * commit * commit * commit * commit * commit * commit * commit * commit * fixed e2e and improved truncate component * fixed pre-commit * comit * comit * commit * fixed unit tests * fixed unit test * commit * Update code --------- Co-authored-by: Kate Golovanova <kate@kgthreads.com> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
* commit * Update pnpm-lock.yaml * Update vite.config.ts * Update package.json * truncate text component * commit * commit * commit * commit * commit * commit * commit * commit * commit * commit * fixed e2e and improved truncate component * fixed pre-commit * comit * comit * commit * fixed unit tests * fixed unit test * commit * Update code --------- Co-authored-by: Kate Golovanova <kate@kgthreads.com> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
3 participants
fixes: #1114