feat(input): adding outside-top prop #4775
Conversation
|
|
@abhinav700 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces a new label placement option, "outside-top," for the Input component. The changes update both the component’s logic and its accompanying documentation. In the component’s code, a new property (isOutsideTop) is added to control label rendering, and the useInput hook’s evaluation logic is modified accordingly. Storybook examples now include the new placement, and theme configurations are updated to support the additional layout style. No public API declarations have been altered. Changes
Sequence Diagram(s)sequenceDiagram
participant IC as Input Component
participant UIH as useInput Hook
participant R as Rendering Logic
IC->>UIH: Call useInput(labelPlacement, placeholder, ...)
UIH->>UIH: Evaluate conditions for isOutsideLeft and isOutsideTop
UIH-->>IC: Return { isOutsideLeft, isOutsideTop, shouldLabelBeOutside }
IC->>R: Render label outside if (isOutsideLeft OR isOutsideTop) else render inside
✨ Finishing Touches
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
|
|
@wingkwong I have created a new branch and added the changes there |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/content/docs/components/input.mdx (1)
84-87: Consider rephrasing the notes to improve readability.The notes provide valuable clarification about label placement behavior, but they could be more concise and avoid repetition.
Consider this revision:
-> **Note**: If the `labelPlacement` is `outside`, `label` is outside only when a placeholder is provided. - -> **Note**: If the `labelPlacement` is `outside-top` or `outside-left`, `label` is outside even if a placeholder is not provided. +> **Note**: Label placement behavior varies: +> - With `outside`: Label appears outside only when a placeholder is provided. +> - With `outside-top` or `outside-left`: Label always appears outside, regardless of placeholder.🧰 Tools
🪛 LanguageTool
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y when a placeholder is provided. > Note: If thelabelPlacementis `outside-...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/docs/content/components/input/label-placements.raw.jsx(1 hunks)apps/docs/content/docs/components/input.mdx(1 hunks)packages/components/input/src/input.tsx(3 hunks)packages/components/input/src/use-input.ts(2 hunks)packages/components/input/stories/input.stories.tsx(3 hunks)packages/core/theme/src/components/input.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/input.mdx
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y when a placeholder is provided. > Note: If the labelPlacement is `outside-...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (6)
apps/docs/content/components/input/label-placements.raw.jsx (1)
4-4: LGTM!The addition of "outside-top" to the placements array is consistent with the new feature.
packages/components/input/src/input.tsx (1)
20-20: LGTM!The implementation correctly handles the new "outside-top" label placement by:
- Adding the
isOutsideTopprop- Updating the label rendering conditions to consider the new placement
Also applies to: 86-86, 119-119
packages/components/input/src/use-input.ts (1)
242-244: LGTM!The implementation correctly:
- Adds variables to track label placement
- Updates the logic to handle the new "outside-top" placement
- Maintains clear documentation of the behavior
Also applies to: 245-250, 260-261
packages/components/input/stories/input.stories.tsx (1)
53-53: LGTM!The changes correctly:
- Add the new placement option to the Storybook configuration
- Include comprehensive examples demonstrating the feature with and without placeholders
Also applies to: 178-182, 187-206
packages/core/theme/src/components/input.ts (1)
181-186: LGTM! Clean implementation of the "outside-top" label placement.The implementation follows the established pattern and uses appropriate Tailwind classes for layout and spacing.
apps/docs/content/docs/components/input.mdx (1)
78-78: LGTM! Clear documentation of the new option.The documentation accurately includes the new "outside-top" option in the list of available label placements.
Searched, but everything seams fine. I think this was a common problem in my previous PR also where I would see this error locally but not while making a PR. |
There was a problem hiding this comment.
@abhinav700 your local branch is outdated. I just push the latest canary branch to your branch. Just FYI, we did some refactoring on label placement recently. Please also update this file and revise your current change if necessary. Thanks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/content/docs/components/input.mdx (1)
86-86: Style Improvement Suggestion foroutside-top/outside-leftNote
This note states that whenlabelPlacementisoutside-toporoutside-left, the label will be displayed outside regardless of a placeholder. However, three successive sentences in this section begin similarly. Consider rephrasing this note to vary the sentence structure and enhance readability.🧰 Tools
🪛 LanguageTool
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y when a placeholder is provided. > Note: If thelabelPlacementis `outside-...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/docs/content/docs/components/input.mdx(1 hunks)packages/components/input/src/use-input.ts(2 hunks)packages/core/theme/src/components/input.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/theme/src/components/input.ts
- packages/components/input/src/use-input.ts
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/input.mdx
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y when a placeholder is provided. > Note: If the labelPlacement is `outside-...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (2)
apps/docs/content/docs/components/input.mdx (2)
78-78: Newoutside-topOption Introduced
The updated text now includes theoutside-topoption along with the existing options. This aligns well with the new feature behavior.
84-84: Clarification onoutsideLabel Behavior
This note clarifies that whenlabelPlacementis set tooutside, the label will only be rendered outside if a placeholder is provided. Ensure that the implementation reflects this conditional logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/system/src/hooks/use-label-placement.ts (1)
12-20: Consider documenting the label placement behavior.The logic handles the new "outside-top" option correctly. Consider adding JSDoc comments to document the behavior of each placement option for better maintainability.
Add documentation above the function:
+/** + * Hook to manage label placement with support for inside, outside, outside-left, and outside-top positions. + * Falls back to "outside" when label is not provided with "inside" placement. + * @param props.labelPlacement - Desired label placement + * @param props.label - Label content + * @returns The resolved label placement + */ export function useLabelPlacement(props: {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/system/src/hooks/use-label-placement.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (1)
packages/core/system/src/hooks/use-label-placement.ts (1)
6-6: LGTM! Verify integration with dependent components.The addition of "outside-top" to the label placement options is a clean, non-breaking change.
Let's verify the integration with dependent components:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for components using labelPlacement ast-grep --pattern 'labelPlacement={$_}'Length of output: 42
🏁 Script executed:
#!/bin/bash # Let's search for the new "outside-top" option and also check for hook usage throughout the codebase. echo "Searching for 'outside-top' usage (string literal)..." rg "\"outside-top\"" . echo "Searching for 'useLabelPlacement' hook usage..." rg "useLabelPlacement" .Length of output: 2470
Integration Verified – "outside-top" Option Properly Propagated
We've confirmed through codebase analysis that the new "outside-top" option is integrated across multiple dependent components (e.g., Input, DateInput, DatePicker, Select) and is properly handled in both hook usage and stories. No issues were found with the hook's implementation or its integration.
|
@wingkwong looks like adding outside top to this causes problems in other packages like date-input |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/input/src/use-input.ts (1)
267-272: Consider simplifying the isLabelOutside condition.The condition can be made more readable by grouping related checks.
- const isLabelOutside = shouldLabelBeOutside - ? isOutsideLeft || - isOutsideTop || - hasPlaceholder || - (labelPlacement === "outside" && hasStartContent) - : false; + const isLabelOutside = shouldLabelBeOutside + ? (isOutsideLeft || isOutsideTop) || // Always outside + (hasPlaceholder || (labelPlacement === "outside" && hasStartContent)) // Conditionally outside + : false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/input/src/use-input.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Build
- GitHub Check: Continuous Release
🔇 Additional comments (3)
packages/components/input/src/use-input.ts (3)
250-252: LGTM! Clear and descriptive variable names.The new variables
isOutsideLeftandisOutsideTopfollow the existing code style and are well-placed in the code.
253-259: LGTM! Well-documented label placement logic.The updated
shouldLabelBeOutsidelogic clearly handles both placeholder-dependent and independent cases, matching the PR objectives.
543-543: LGTM! Consistent return object update.The addition of
isOutsideTopto the return object follows the existing pattern and is required for component rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/input/src/use-input.ts (1)
231-261: Implementation looks good, but consider improving error handling.The implementation correctly handles file input edge cases and provides appropriate warnings. However, consider adding validation for other unsupported label placements.
const labelPlacement = useMemo<InputVariantProps["labelPlacement"]>(() => { + const supportedPlacements = ["inside", "outside", "outside-left", "outside-top"]; + if (originalProps.labelPlacement && !supportedPlacements.includes(originalProps.labelPlacement)) { + warn(`Unsupported label placement "${originalProps.labelPlacement}". Using default...`); + return "inside"; + } if (isFileTypeInput) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/input/src/use-input.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Build
- GitHub Check: Continuous Release
🔇 Additional comments (3)
packages/components/input/src/use-input.ts (3)
272-280: LGTM! Clear and concise implementation of label placement logic.The new variables and logic for determining label placement are well-structured and easy to understand.
564-565: Ensure the new property is properly exposed.The
isOutsideTopproperty is correctly added to the hook's return value, making it available for consumers.
289-294: Verify the impact on existing label placement behavior.The modified label outside check includes the new
outside-topoption. Please ensure this change doesn't affect existing components that rely on the label placement logic.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for components using label placement ast-grep --pattern 'labelPlacement={$_}'Length of output: 42
🏁 Script executed:
#!/bin/bash # Search for all occurrences of "labelPlacement" in the repository to understand its usage rg "labelPlacement" -nLength of output: 34123
Label Placement Behavior Verified for "outside-top"
Based on our review and the repository search results:
• The "outside-top" option is explicitly used in component stories and documented in the examples.
• The new check in use-input.ts now correctly includes a condition for "outside-top" without interfering with the legacy "outside" or "outside-left" behaviors.
• All usages in the codebase align with the intended label placement logic, so no adverse impact on existing components is expected.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -82,7 +83,7 @@ const Input = forwardRef<"input", InputProps>((props, ref) => { | |||
| return ( | |||
| <div {...getMainWrapperProps()}> | |||
| <div {...getInputWrapperProps()}> | |||
| {!isOutsideLeft ? labelContent : null} | |||
| {!isOutsideLeft && !isOutsideTop ? labelContent : null!} | |||
There was a problem hiding this comment.
maybe added by mistake. I don't remember adding it
| /** | ||
| * Following code is not used because adding 'outside-top' to | ||
| * 'use-label-placement' hook breaks other component. | ||
| * Folowing code is replaced with the old way of computing | ||
| * 'labelPlacement' for 'input' component. | ||
| * | ||
| * const labelPlacement = useLabelPlacement({ | ||
| * labelPlacement: originalProps.labelPlacement, | ||
| * label, | ||
| * }); | ||
| */ | ||
|
|
There was a problem hiding this comment.
// TODO: not using useLabelPlacement because `outside-top` doesn't support in other components.| if (isFileTypeInput) { | ||
| // if `labelPlacement` is not defined, choose `outside` instead | ||
| // since the default value `inside` is not supported in file input | ||
| if (!originalProps.labelPlacement) return "outside"; | ||
| // throw a warning if `labelPlacement` is `inside` | ||
| // and change it to `outside` | ||
| if (originalProps.labelPlacement === "inside") { | ||
| warn("Input with file type doesn't support inside label. Converting to outside ..."); | ||
|
|
||
| return "outside"; | ||
| } | ||
| } |
There was a problem hiding this comment.
These lines have been deleted in latest canary.
This PR is duplicate of #3660
PR #4761 is closed because the old branch is deleted
📝 Description
⛳️ Current behavior (updates)
In old behaviour, the label was presented inside the input component if placholder === "" even if labelPlacement === "outside"
🚀 New behavior
Adding outside-top prop which dispalys the label outside the input component regardless of whether the placeholder is there or not.
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit