fix: div-in-p issue in tooltip #4053
Conversation
|
@rishabhverma54321 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
WalkthroughThe changes in this pull request enhance the Changes
Possibly related issues
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (2)
packages/components/tooltip/src/tooltip.tsx (1)
46-47: Consider adding ARIA attributes for better accessibilityWhile the div wrapper solves the hydration issue, consider adding appropriate ARIA attributes to maintain semantic meaning, especially since we're using a generic div container.
- trigger = <div {...getTriggerProps()}>{children}</div>; + trigger = <div {...getTriggerProps()} role="tooltip" aria-label="tooltip content">{children}</div>;packages/components/tooltip/src/use-tooltip.ts (1)
94-94: Consider narrowing the LazyElement type definitionThe current type definition includes
nullandundefined, which might be too permissive for lazy elements. Consider if these should be handled separately to make the type more precise.-export type LazyElement = ReactElement | ReactNode | null | undefined; +export type LazyElement = ReactElement | ReactNode;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/components/tooltip/src/tooltip.tsx(2 hunks)packages/components/tooltip/src/use-tooltip.ts(3 hunks)
🔇 Additional comments (4)
packages/components/tooltip/src/tooltip.tsx (2)
10-10: LGTM: Import change aligns with new functionality
The addition of isLazyElement import is necessary for the new lazy element detection feature.
44-47: Implementation correctly addresses hydration issues
The changes effectively solve the div-in-p hydration error by:
- Adding a check for lazy elements
- Using appropriate container elements (div for lazy components, p for text)
Let's verify the styling consistency:
✅ Verification successful
Implementation is safe and styling is properly handled
After reviewing the implementation, the change to use a div wrapper for lazy elements is safe because:
- Styling is handled through the
getTriggerProps()which applies consistent classes and attributes - The tooltip uses a slot-based styling system that's independent of the trigger element type
- The trigger wrapper (p or div) inherits the same props and styling mechanism regardless of the element type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing tooltip-specific CSS that might be affected by the div wrapper
rg -g '*.{css,scss}' 'tooltip'
# Look for potential style overrides that might affect the new div wrapper
ast-grep --pattern 'div[class*="tooltip"]'
Length of output: 75
Script:
#!/bin/bash
# Let's try a broader search for tooltip-related styles
rg -g '*.{css,scss,styled.ts,styles.ts}' -i 'tooltip'
# Check for styled-components usage in tooltip component
rg -t typescript -t typescriptreact "styled" packages/components/tooltip/
# Look for any CSS modules or style imports in tooltip
rg -t typescript -t typescriptreact "import.*style|import.*css" packages/components/tooltip/
Length of output: 292
Script:
#!/bin/bash
# Search for tooltip styles with correct ripgrep file extensions
rg -g '*.{css,scss,ts,tsx}' -i 'tooltip.*{style|css}'
# Check the tooltip component file itself for styling approach
cat packages/components/tooltip/src/tooltip.tsx
# Look for any style-related imports or styled-components usage
rg -g '*.{ts,tsx}' "import.*styled|import.*css|import.*style" packages/components/tooltip/
Length of output: 3340
Script:
#!/bin/bash
# Check the Component prop type and usage
cat packages/components/tooltip/src/use-tooltip.ts
# Look for any default styles or className props being passed
rg -g '*.{ts,tsx}' "getTooltipProps|getTriggerProps" packages/components/tooltip/
Length of output: 9261
packages/components/tooltip/src/use-tooltip.ts (2)
7-7: LGTM: Clean import addition
The ReactElement import is appropriately added to support the new LazyElement type.
299-310: 🛠️ Refactor suggestion
Add support for server components
To fully address the PR objective of handling both lazy and server components, consider adding server component detection.
Let's verify if server components have a similar identifier:
export function isLazyElement(element: LazyElement): boolean {
if (typeof element !== "object" || element === null) {
return false;
}
// Check if it's a ReactElement and has the $$typeof property of React.lazy
const reactElement = element as ReactNode & {
$$typeof: symbol;
};
- return reactElement.$$typeof === Symbol.for("react.lazy");
+ return (
+ reactElement.$$typeof === Symbol.for("react.lazy") ||
+ reactElement.$$typeof === Symbol.for("react.server.reference")
+ );
}| export function isLazyElement(element: LazyElement): boolean { | ||
| if (typeof element !== "object" || element === null) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check if it's a ReactElement and has the $$typeof property of React.lazy | ||
| const reactElement = element as ReactNode & { | ||
| $$typeof: symbol; | ||
| }; | ||
|
|
||
| return reactElement.$$typeof === Symbol.for("react.lazy"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation and improve type safety
The function implementation is correct but could benefit from better documentation and type safety.
- Add JSDoc documentation:
+/**
+ * Checks if the given element is a React lazy component.
+ * @param element - The element to check
+ * @returns true if the element is a React lazy component, false otherwise
+ */
export function isLazyElement(element: LazyElement): boolean {- Make it a type guard for better type safety:
-export function isLazyElement(element: LazyElement): boolean {
+export function isLazyElement(element: LazyElement): element is ReactElement {📝 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 function isLazyElement(element: LazyElement): boolean { | |
| if (typeof element !== "object" || element === null) { | |
| return false; | |
| } | |
| // Check if it's a ReactElement and has the $$typeof property of React.lazy | |
| const reactElement = element as ReactNode & { | |
| $$typeof: symbol; | |
| }; | |
| return reactElement.$$typeof === Symbol.for("react.lazy"); | |
| } | |
| /** | |
| * Checks if the given element is a React lazy component. | |
| * @param element - The element to check | |
| * @returns true if the element is a React lazy component, false otherwise | |
| */ | |
| export function isLazyElement(element: LazyElement): element is ReactElement { | |
| if (typeof element !== "object" || element === null) { | |
| return false; | |
| } | |
| // Check if it's a ReactElement and has the $$typeof property of React.lazy | |
| const reactElement = element as ReactNode & { | |
| $$typeof: symbol; | |
| }; | |
| return reactElement.$$typeof === Symbol.for("react.lazy"); | |
| } |
Closes #
📝 Description
This PR adds support for using React lazy or server components inside a Tooltip component
⛳️ Current behavior (updates)
Currently, in Next 14 and above, passing a lazy or server component inside a tooltip results in an error.
In HTML, <div> cannot be a descendant of <p>. This will cause a hydration error.🚀 New behavior
A new check has been added to render lazy and server components in a div instead of a p tag to prevent hydration issues.
💣 Is this a breaking change (Yes/No):
No - This condition will only work with lazy and server components.
📝 Additional Information
Summary by CodeRabbit
New Features
Tooltipcomponent to support lazy-loaded React elements.childrenprop to ensure it is a valid React element or a lazy element.Bug Fixes
children, defaulting to a<p>element when necessary.Documentation