Feature Tooltip: support react node - #169
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated tooltip/label implementation introduces a likely behavior regression for ReactNode tooltip content, a potentially incorrect Label prop (htmlFor vs for), and a new circular module dependency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the form components’ label tooltip API to accept richer content (ReactNode) instead of only strings, and adjusts the label rendering logic accordingly.
Changes:
- Widen
labelToolTiptype fromstringtoReactNode. - Refactor
FormGroupLayoutLabeltooltip rendering into a dedicatedTooltiphelper and update prop wiring. - Document the change in
CHANGELOG.md.
File summaries
| File | Description |
|---|---|
| src/lib/types/CommonInputProps.ts | Widen labelToolTip to ReactNode in shared input props. |
| src/lib/FormGroupLayoutLabel.tsx | Refactors tooltip rendering and label prop wiring. |
| src/lib/FormGroupLayout.tsx | Updates prop name passed into FormGroupLayoutLabel. |
| CHANGELOG.md | Notes the new labelToolTip capability. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
commit: |
There was a problem hiding this comment.
🔵 Needs a closer look
The current implementation can break <Label>’s input association (htmlFor vs for) and renders non-string labelToolTip inline instead of as tooltip content, which undermines the intended feature.
Review details
Suppressed comments (3)
src/lib/FormGroupLayoutLabel.tsx:19
- When
labelToolTipis a non-string ReactNode (the new supported type), this returns the node inline and skips rendering the tooltip icon +UncontrolledTooltip, so consumers won't get a tooltip at all. Consider always rendering the icon/UncontrolledTooltipand usinglabelToolTipas the tooltip content.
if (!isTextTooltip) {
return labelToolTip;
}
src/lib/FormGroupLayoutLabel.tsx:70
@neolution-ch/reactstrap's<Label>usage in this repo consistently uses theforprop (e.g.src/lib/Input.tsx:130). Switching tohtmlForhere may not be supported by theLabelcomponent and can break the label→input association.
<Label check={checkboxLayout || switchLayout} htmlFor={fieldId} style={labelStyle}>
CHANGELOG.md:12
- Changelog entry is grammatically unclear; consider using the React type name
ReactNodeto describe the change.
- support to react node for `labelToolTip`
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new ReactNode tooltip path currently bypasses tooltip rendering for non-string values and the Label prop change (htmlFor vs for) may break established library usage.
Review details
Suppressed comments (3)
src/lib/FormGroupLayoutLabel.tsx:21
labelToolTipis now typed asReactNode, but non-string values are returned directly (rendered inline) and skip the tooltip icon +UncontrolledTooltip. This means passing rich content (e.g.<><b>…</b></>) will not show as a tooltip. Consider always rendering the icon +UncontrolledTooltipand letting its content be aReactNode.
if (!isTextTooltip) {
return labelToolTip;
}
return (
src/lib/FormGroupLayoutLabel.tsx:70
Labelfrom@neolution-ch/reactstrapis used elsewhere with theforprop (e.g.src/lib/Input.tsx:130). Switching tohtmlForhere may not wire up the label correctly if the component only mapsfor->htmlFor.
<Label check={checkboxLayout || switchLayout} for={fieldId} style={labelStyle}>
CHANGELOG.md:12
- Changelog entry wording is unclear and the type name should be
ReactNode(capitalized) to match the public API.
- support to react node for `labelToolTip`
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new tooltip rendering logic currently bypasses the tooltip UI for non-string ReactNode values, which undermines the stated goal of supporting ReactNode tooltip content.
Review details
Suppressed comments (2)
src/lib/FormGroupLayoutLabel.tsx:19
labelToolTipis now typed asReactNode, but theTooltipcomponent only shows the tooltip UI when the value is a string; for any non-string ReactNode it renders the node inline instead of inside the tooltip. This breaks the expected behavior when passing JSX as tooltip content and doesn’t match the PR goal of supporting ReactNode tooltip content.
const isTextTooltip = typeof labelToolTip === "string";
if (!labelToolTip) {
return null;
}
if (!isTextTooltip) {
return labelToolTip;
}
CHANGELOG.md:12
- Changelog entry grammar is unclear; it reads like a sentence fragment. Consider updating it to explicitly mention
ReactNodesupport.
- support to react node for `labelToolTip`
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The current FormGroupLayoutLabel implementation does not display a tooltip for non-string ReactNode values (it renders them inline) and uses truthiness checks that can drop valid ReactNode values (e.g., 0).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/lib/FormGroupLayoutLabel.tsx:57
- The tooltip-without-label guard uses truthiness (
!!labelToolTip), which won’t catch some valid ReactNodes (e.g.0) and is inconsistent with the Tooltip null-check. Use a nullish check instead.
if (!label && !!labelToolTip) {
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
ReactNode labelToolTip values currently bypass the tooltip rendering and will display inline instead of in a hover tooltip.
Review details
Suppressed comments (2)
src/lib/FormGroupLayoutLabel.tsx:18
labelToolTipis now typed asReactNode, but non-string values are returned directly, bypassing the tooltip icon +UncontrolledTooltip. This makes ReactNode tooltips render inline instead of in a hover tooltip.
if (typeof labelToolTip !== "string") {
return labelToolTip;
}
CHANGELOG.md:12
- Changelog entry grammar/casing is off; it should refer to
ReactNodeand read naturally.
- support to react node for `labelToolTip`
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Feature Tooltip: support react node