feat(OverflowTooltipText): add new component #1461
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/react-components/src/components/OverflowTooltipText/types.ts (1)
1-3: Consider adding common tooltip customization propsThe interface could benefit from additional props like:
placement?: 'top' | 'bottom' | 'left' | 'right'showDelay?: numberclassName?: stringpackages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.module.scss (1)
1-3: Consider adding max-width to tooltip contentTo prevent excessively wide tooltips, consider adding a max-width constraint:
.tooltipContent { word-break: break-word; + max-width: 300px; }packages/react-components/src/hooks/useOnHover.ts (1)
5-16: Consider memoizing event handlersThe implementation is clean, but handlers could be memoized with useCallback to prevent unnecessary re-renders in child components.
export const useOnHover = (initialState = false): IUseOnHover => { const [isHovered, setIsHovered] = useState(initialState); - const handleMouseOver = (): void => setIsHovered(true); - const handleMouseOut = (): void => setIsHovered(false); + const handleMouseOver = useCallback((): void => setIsHovered(true), []); + const handleMouseOut = useCallback((): void => setIsHovered(false), []); return { isHovered, handleMouseOver, handleMouseOut, }; };packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.tsx (1)
29-38: Consider memoizing the tooltip contentFor performance optimization with long text strings:
+ const tooltipContent = useMemo( + () => <div className={styles.tooltipContent}>{text}</div>, + [text] + ); return ( <Tooltip kind="invert" isVisible={isOverflow && isHovered} - triggerRenderer={renderChildren} + triggerRenderer={() => children} > - <div className={styles.tooltipContent}>{text}</div> + {tooltipContent} </Tooltip> );packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.mdx (1)
13-15: Enhance documentation with accessibility and more examplesConsider adding:
- Accessibility considerations
- Examples with different text lengths
- Use cases with various container widths
packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.stories.tsx (2)
9-17: Consider using a more descriptive class name.The class name "text-container" could be more specific, like "overflow-tooltip-demo-container" to better indicate its purpose.
19-73: Consider adding more edge cases.Great coverage of basic scenarios! Consider adding stories for:
- Different container widths (responsive behavior)
- RTL text support
- HTML content within the tooltip
export const RTLText = { args: { text: 'مرحبا بكم في هذا النص العربي الطويل الذي يتجاوز حدود الحاوية', }, }; export const ResponsiveContainer = { render: (args: OverflowTooltipTextProps) => ( <div style={{ width: '50%' }}> <OverflowTooltipText {...args} /> </div> ), args: { text: 'This text will respond to container width changes', }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.mdx(1 hunks)packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.module.scss(1 hunks)packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.stories.css(1 hunks)packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.stories.tsx(1 hunks)packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.tsx(1 hunks)packages/react-components/src/components/OverflowTooltipText/index.ts(1 hunks)packages/react-components/src/components/OverflowTooltipText/types.ts(1 hunks)packages/react-components/src/hooks/index.ts(1 hunks)packages/react-components/src/hooks/types.ts(2 hunks)packages/react-components/src/hooks/useIsOverflow.ts(1 hunks)packages/react-components/src/hooks/useOnHover.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.stories.css
- packages/react-components/src/components/OverflowTooltipText/index.ts
🔇 Additional comments (8)
packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.module.scss (1)
5-9: LGTM!
Text truncation styles follow best practices.
packages/react-components/src/hooks/index.ts (1)
5-6: LGTM!
Hook exports follow consistent naming and export patterns.
packages/react-components/src/hooks/types.ts (2)
3-3: Good improvement on NODE type
Broadening to HTMLElement makes the type more reusable across different HTML elements.
30-34: LGTM! Clean interface definition
Interface is well-structured with clear purpose and typed handlers.
packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.tsx (1)
1-9: Clean imports with good separation of concerns!
The imports are well-organized, using named imports and custom hooks for specific functionality.
packages/react-components/src/index.ts (1)
37-37: Export is correctly placed and follows the existing pattern!
packages/react-components/src/components/OverflowTooltipText/OverflowTooltipText.stories.tsx (2)
1-8: LGTM! Clean and well-organized imports.
1-73: Verify consistency with design system patterns.
Let's ensure the component follows similar patterns as other components in the design system.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Component follows design system patterns correctly
The component follows the established patterns in the design system:
- Uses Typography components (Text, Heading) consistently like other components
- Story structure matches other components with similar title format and render patterns
- Follows the same import patterns for Typography as seen across 50+ other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other components follow similar patterns
echo "Checking component structure..."
fd -e tsx -e ts "^[A-Z].*\.stories\.(tsx|ts)$" packages/react-components/src/components -x cat {} \; | grep -A 5 "^export default"
# Check Typography component usage patterns
echo "Checking Typography usage..."
rg -A 3 "import.*Typography" packages/react-components/src/components
Length of output: 34564
| const checkOverflowDebounced = debounce(() => { | ||
| checkOverflow(); | ||
| }, 100); |
There was a problem hiding this comment.
Clean up debounced function on unmount
The debounced function should be cancelled in the cleanup function to prevent memory leaks.
const checkOverflowDebounced = debounce(() => {
checkOverflow();
}, 100);
+
+useEffect(() => {
+ return () => {
+ checkOverflowDebounced.cancel();
+ };
+}, [checkOverflowDebounced]);📝 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.
| const checkOverflowDebounced = debounce(() => { | |
| checkOverflow(); | |
| }, 100); | |
| const checkOverflowDebounced = debounce(() => { | |
| checkOverflow(); | |
| }, 100); | |
| useEffect(() => { | |
| return () => { | |
| checkOverflowDebounced.cancel(); | |
| }; | |
| }, [checkOverflowDebounced]); |
| useLayoutEffect(() => { | ||
| checkOverflow(); | ||
|
|
||
| const node = ref.current; | ||
| if (node) { | ||
| resizeCallback(node, () => { | ||
| checkOverflowDebounced(); | ||
| }); | ||
| } | ||
|
|
||
| return () => { | ||
| resizeCallback(null, () => {}); | ||
| }; | ||
| }, [ref, checkOverflowDebounced]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize useLayoutEffect dependencies
The effect's dependency array includes checkOverflowDebounced which changes on every render due to debounce creating a new function.
- }, [ref, checkOverflowDebounced]);
+ }, [ref, checkOverflow]); // Use checkOverflow insteadAlso, consider using a cleanup pattern that's more resilient to race conditions:
useLayoutEffect(() => {
+ let mounted = true;
checkOverflow();
const node = ref.current;
if (node) {
resizeCallback(node, () => {
+ if (mounted) {
checkOverflowDebounced();
+ }
});
}
return () => {
+ mounted = false;
resizeCallback(null, () => {});
};
}, [ref, checkOverflow]);📝 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.
| useLayoutEffect(() => { | |
| checkOverflow(); | |
| const node = ref.current; | |
| if (node) { | |
| resizeCallback(node, () => { | |
| checkOverflowDebounced(); | |
| }); | |
| } | |
| return () => { | |
| resizeCallback(null, () => {}); | |
| }; | |
| }, [ref, checkOverflowDebounced]); | |
| useLayoutEffect(() => { | |
| let mounted = true; | |
| checkOverflow(); | |
| const node = ref.current; | |
| if (node) { | |
| resizeCallback(node, () => { | |
| if (mounted) { | |
| checkOverflowDebounced(); | |
| } | |
| }); | |
| } | |
| return () => { | |
| mounted = false; | |
| resizeCallback(null, () => {}); | |
| }; | |
| }, [ref, checkOverflow]); |
| export const OverflowTooltipText: FC<OverflowTooltipTextProps> = ({ text }) => { | ||
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
| const isOverflow = useIsOverflow(wrapperRef); | ||
| const { isHovered, handleMouseOut, handleMouseOver } = useOnHover(isOverflow); | ||
|
|
||
| const renderChildren = () => { | ||
| return ( | ||
| <div | ||
| onMouseEnter={handleMouseOver} | ||
| onMouseLeave={handleMouseOut} | ||
| ref={wrapperRef} | ||
| className={styles.text} | ||
| > | ||
| {text} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and consider simplifying the code
The component needs accessibility improvements and could be simplified.
Consider these changes:
- const renderChildren = () => {
- return (
- <div
- onMouseEnter={handleMouseOver}
- onMouseLeave={handleMouseOut}
- ref={wrapperRef}
- className={styles.text}
- >
- {text}
- </div>
- );
- };
+ const children = (
+ <div
+ onMouseEnter={handleMouseOver}
+ onMouseLeave={handleMouseOut}
+ ref={wrapperRef}
+ className={styles.text}
+ aria-label={text}
+ >
+ {text}
+ </div>
+ );📝 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.
| export const OverflowTooltipText: FC<OverflowTooltipTextProps> = ({ text }) => { | |
| const wrapperRef = useRef<HTMLDivElement>(null); | |
| const isOverflow = useIsOverflow(wrapperRef); | |
| const { isHovered, handleMouseOut, handleMouseOver } = useOnHover(isOverflow); | |
| const renderChildren = () => { | |
| return ( | |
| <div | |
| onMouseEnter={handleMouseOver} | |
| onMouseLeave={handleMouseOut} | |
| ref={wrapperRef} | |
| className={styles.text} | |
| > | |
| {text} | |
| </div> | |
| ); | |
| }; | |
| export const OverflowTooltipText: FC<OverflowTooltipTextProps> = ({ text }) => { | |
| const wrapperRef = useRef<HTMLDivElement>(null); | |
| const isOverflow = useIsOverflow(wrapperRef); | |
| const { isHovered, handleMouseOut, handleMouseOver } = useOnHover(isOverflow); | |
| const children = ( | |
| <div | |
| onMouseEnter={handleMouseOver} | |
| onMouseLeave={handleMouseOut} | |
| ref={wrapperRef} | |
| className={styles.text} | |
| aria-label={text} | |
| > | |
| {text} | |
| </div> | |
| ); | |
Resolves: #1458
Description
This pull request introduces the
OverflowTooltipTextcomponent to thereact-componentspackage. The component displays a text element with a tooltip that appears when the text overflows its container, enhancing readability and providing additional context in space-constrained layouts. The changes include the implementation of the component and necessary hooks.Hooks:
packages/react-components/src/hooks/useIsOverflow.ts: Implemented theuseIsOverflowhook to check if the text overflows its container.packages/react-components/src/hooks/useOnHover.ts: Implemented theuseOnHoverhook to manage hover state.Storybook
https://feature-1458--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
New Features
OverflowTooltipTextcomponent for displaying text with tooltips when overflowing.OverflowTooltipTextcomponent.Style
Chores
OverflowTooltipTextcomponent for easier imports.