feat: global labelPlacement prop #4346
Conversation
🦋 Changeset detectedLatest commit: 03e9405 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a series of enhancements across various components from the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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
|
macci001
force-pushed
the
macci001/eng-1694
branch
from
3f1deed to
4457d0e
Compare
macci001
force-pushed
the
macci001/eng-1694
branch
from
4457d0e to
21b8328
Compare
macci001
force-pushed
the
macci001/eng-1694
branch
from
21b8328 to
45dbecf
Compare
macci001
force-pushed
the
macci001/eng-1694
branch
from
45dbecf to
5fffcdd
Compare
![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 (5)
.changeset/chilly-dancers-switch.md (1)
9-10: Enhance the changeset descriptionThe current description could be more informative. Consider adding:
- Available values for labelPlacement (inside, outside, outside-left)
- Impact on affected components
- Migration notes if any
Also, add a space between "prop" and the ticket reference.
Here's a suggested improvement:
-Adding support for global labelPlacement prop.(ENG-1694) +Adding support for global labelPlacement prop to customize label positioning across components. + +The labelPlacement prop can be set globally via NextUIProvider with the following values: +- inside +- outside +- outside-left + +Affects: Input, Select, DatePicker, DateInput, and related components. + +(ENG-1694)🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Possible missing comma found.
Context: ...": patch --- Adding support for global labelPlacement prop.(ENG-1694)(AI_HYDRA_LEO_MISSING_COMMA)
packages/core/system/src/provider-context.ts (1)
14-19: Enhance JSDoc documentation for labelPlacementWhile the documentation is present, it would be more helpful to include:
- Description of each possible value
- Example usage
- Default behavior implications
Consider expanding the documentation like this:
/** * Position where the label should appear. * + * @description + * - "inside": Label appears within the input field + * - "outside": Label appears above the input field + * - "outside-left": Label appears to the left of the input field + * + * @example + * ```tsx + * <NextUIProvider labelPlacement="outside"> + * <Input label="Username" /> + * </NextUIProvider> + * ``` * * @default undefined */apps/docs/content/docs/api-references/nextui-provider.mdx (1)
145-150: Enhance the documentation for thelabelPlacementprop.The documentation could be more detailed to help developers understand the impact of each value:
`labelPlacement` - - **Description**: Determines the position where label should appear, such as inside, outside or outside-left of the component. + - **Description**: Determines the position where label should appear in form components: + - `inside`: Places the label inside the input field, floating when focused or filled + - `outside`: Places the label above the input field + - `outside-left`: Places the label to the left of the input field + - `undefined`: Uses the default placement for each component - - **Type**: `string` | `undefined` - - **Possible Values**: `inside` | `outside` | `outside-left` | `undefined` + - **Type**: `"inside" | "outside" | "outside-left" | undefined` - **Default**: `undefined`packages/components/date-input/src/use-time-input.ts (1)
137-145: Optimize theuseMemodependency array.Consider destructuring the required properties earlier to make the dependencies more explicit and maintainable.
+ const globalLabelPlacement = globalContext?.labelPlacement; + const labelPlacement = useMemo<DateInputVariantProps["labelPlacement"]>(() => { const labelPlacement = - originalProps.labelPlacement ?? globalContext?.labelPlacement ?? "inside"; + originalProps.labelPlacement ?? globalLabelPlacement ?? "inside"; if (labelPlacement === "inside" && !label) { return "outside"; } return labelPlacement; - }, [originalProps.labelPlacement, globalContext?.labelPlacement, label]); + }, [originalProps.labelPlacement, globalLabelPlacement, label]);packages/components/date-picker/src/use-date-picker-base.ts (1)
12-12: Consider importing from a shared location.The
useMemoimport could be grouped with other React hooks if they're imported from the same location.-import {useCallback, useMemo} from "react"; +import {useCallback, useMemo, type ReactNode} from "react";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.changeset/chilly-dancers-switch.md(1 hunks)apps/docs/content/docs/api-references/nextui-provider.mdx(1 hunks)packages/components/date-input/src/use-date-input.ts(1 hunks)packages/components/date-input/src/use-time-input.ts(1 hunks)packages/components/date-picker/src/use-date-picker-base.ts(2 hunks)packages/components/date-picker/src/use-date-range-picker.ts(4 hunks)packages/components/input/src/use-input.ts(2 hunks)packages/components/select/src/use-select.ts(1 hunks)packages/core/system/src/provider-context.ts(1 hunks)packages/core/system/src/provider.tsx(3 hunks)packages/core/theme/src/components/date-input.ts(0 hunks)packages/core/theme/src/components/input.ts(0 hunks)packages/core/theme/src/components/select.ts(0 hunks)packages/storybook/.storybook/preview.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- packages/core/theme/src/components/date-input.ts
- packages/core/theme/src/components/input.ts
- packages/core/theme/src/components/select.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/chilly-dancers-switch.md
[uncategorized] ~10-~10: Possible missing comma found.
Context: ...": patch --- Adding support for global labelPlacement prop.(ENG-1694)
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (11)
.changeset/chilly-dancers-switch.md (1)
1-8: LGTM: Version bumps are appropriate
The patch version bumps are correctly specified for all affected components, which aligns with the non-breaking nature of the changes.
packages/storybook/.storybook/preview.tsx (2)
10-10: LGTM: Proper integration of labelPlacement in Storybook decorator
The labelPlacement property is correctly destructured from globals and properly passed to the NextUIProvider.
Also applies to: 16-16
130-141: LGTM: Well-structured Storybook toolbar configuration
The labelPlacement toolbar configuration is well-implemented with clear title, description, and appropriate options matching the provider's supported values.
packages/core/system/src/provider.tsx (1)
61-61: LGTM: Clean implementation of labelPlacement in provider
The labelPlacement property is properly:
- Destructured from props
- Included in the memoized context
- Added to the dependency array
Also applies to: 89-89, 98-98
packages/components/date-input/src/use-date-input.ts (1)
195-203: LGTM: Clean implementation of labelPlacement logic
The implementation correctly handles the label placement hierarchy:
- Uses prop value if provided
- Falls back to global context value
- Defaults to "inside"
- Automatically switches to "outside" when no label is present
packages/components/input/src/use-input.ts (2)
234-242: LGTM: Proper handling of file input edge case
The code correctly handles the special case for file inputs by defaulting to "outside" placement and providing a clear warning when the global context attempts to use "inside" placement.
253-261: LGTM: Consistent implementation of labelPlacement logic
The implementation aligns with the standard pattern used across components:
- Props take precedence
- Falls back to global context
- Uses "inside" as default
- Automatically switches to "outside" when no label is present
packages/components/select/src/use-select.ts (1)
347-355: LGTM: Consistent implementation of labelPlacement logic
The implementation maintains consistency with other components by following the same pattern:
- Props take precedence
- Falls back to global context
- Uses "inside" as default
- Automatically switches to "outside" when no label is present
packages/components/date-picker/src/use-date-range-picker.ts (3)
63-63: LGTM: Label parameter addition is well-integrated
The addition of the label parameter to the hook's props is consistent with the PR's objective and maintains backward compatibility.
399-399: LGTM: Proper label prop handling
The label prop is correctly forwarded to the input group and exposed in the hook's return value, following React best practices.
Also applies to: 427-427
148-156: Verify the automatic label placement behavior
The logic forces outside placement when there's no label, even if inside was explicitly specified. This automatic override might:
- Cause unexpected behavior for consumers
- Lead to layout shifts if the label changes dynamically
- Conflict with explicit placement preferences
Consider:
- Documenting this behavior
- Making it configurable
- Respecting the explicit placement preference
✅ Verification successful
The automatic label placement behavior is consistent across components
The behavior of forcing outside placement when there's no label is a deliberate design pattern implemented consistently across multiple form components:
- Input component (
use-input.ts) - Select component (
use-select.ts) - Date/Time inputs (
use-date-input.ts,use-time-input.ts) - Date pickers (
use-date-picker-base.ts,use-date-range-picker.ts)
This consistent implementation suggests it's an intentional UX decision to prevent empty inside labels that could cause layout issues or poor user experience. The behavior is also handled uniformly through the NextUI Provider's global context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar behavior in other components
ast-grep --pattern 'if ($placement === "inside" && !$label) {
return "outside";
}'
Length of output: 86
Script:
#!/bin/bash
# Search for similar behavior with labelPlacement in other components
rg -A 3 'labelPlacement.*=.*"inside"' --type ts
# Search for any label placement logic
rg -A 3 'labelPlacement' --type ts
# Search for documentation about label placement
fd README.md | xargs rg -i 'label.*placement|placement.*label'
Length of output: 67972
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/system/src/hooks/use-label-placement.ts (1)
12-20: Consider adding JSDoc documentation.The implementation logic is solid with proper fallback handling and edge cases. However, adding JSDoc documentation would help explain the behavior, especially the special case where "inside" placement defaults to "outside" when no label is provided.
+/** + * Custom hook to determine label placement based on props and global context. + * @param props.labelPlacement - Optional label placement override + * @param props.label - Optional label content + * @returns The computed label placement, defaulting to "outside" if placement is "inside" with no label + */ export function useLabelPlacement(props: {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/components/date-input/src/use-date-input.ts(2 hunks)packages/components/date-input/src/use-time-input.ts(2 hunks)packages/components/date-picker/src/use-date-picker-base.ts(2 hunks)packages/components/date-picker/src/use-date-range-picker.ts(5 hunks)packages/components/select/src/use-select.ts(2 hunks)packages/core/system/src/hooks/index.ts(1 hunks)packages/core/system/src/hooks/use-label-placement.ts(1 hunks)packages/core/system/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/system/src/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/date-input/src/use-time-input.ts
- packages/components/date-picker/src/use-date-picker-base.ts
- packages/components/select/src/use-select.ts
- packages/components/date-input/src/use-date-input.ts
- packages/components/date-picker/src/use-date-range-picker.ts
🔇 Additional comments (3)
packages/core/system/src/hooks/use-label-placement.ts (2)
5-8: LGTM! Well-defined props interface.
The props interface is clear and type-safe, using proper TypeScript types for labelPlacement and label.
9-10: Verify error handling for missing provider context.
The hook accesses globalContext without null checks. While the optional chaining on line 10 prevents runtime errors, we should verify this behavior is intentional.
packages/core/system/src/index.ts (1)
36-37: LGTM! Clean export addition.
The useLabelPlacement hook export follows the established pattern and maintains consistency with other exports.
macci001
force-pushed
the
macci001/eng-1694
branch
from
90081ae to
cc685d5
Compare
macci001
force-pushed
the
macci001/eng-1694
branch
from
cc685d5 to
1a0a128
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/components/spacer/package.json (1)
39-39: Consider documenting the dependency update rationaleWhile the Spacer component might not directly use the labelPlacement feature, it requires the updated theme package. Consider documenting this indirect dependency requirement in the changelog.
This update pattern across components suggests a need for a centralized dependency management strategy. Consider using a tool like Renovate or Dependabot to automate version bumps across packages.
packages/components/kbd/package.json (1)
39-39: Version update preserves compatibilityThe peer dependency update is appropriate and maintains consistency with other components while preserving backward compatibility through the use of the >= operator.
For package consumers: This update requires a minimum theme version of 2.4.3, but it's recommended to update all NextUI packages together to maintain consistency and avoid potential version conflicts.
packages/components/link/package.json (1)
39-40: Version updates are aligned with the global changes.The peer dependency updates maintain consistency:
- @nextui-org/theme: >=2.4.3
- @nextui-org/system: >=2.4.5
Consider adding a note in the changelog or documentation about the minimum version requirements for the new labelPlacement feature.
packages/components/breadcrumbs/package.json (1)
39-40: Consider selective version updates based on component needsThe current approach updates all component dependencies uniformly. Consider:
- Only updating versions for components that actually use the labelPlacement feature
- Creating a dependency map to track which components require these updates
- Documenting the minimum version requirements in the PR description
This would help maintain looser version constraints where possible and make future updates easier to manage.
packages/components/tooltip/package.json (1)
40-41: LGTM! Consider updating the changelogThe peer dependency updates are consistent with other components. Since this is part of a feature rollout (global labelPlacement), consider documenting these version requirements in the changelog.
packages/components/modal/package.json (1)
37-41: Consider documenting the minimum version requirementsSince these version updates are related to the new global labelPlacement feature, consider:
- Adding a note in the migration guide about minimum version requirements
- Updating the documentation to reflect these version dependencies
- Adding examples of using the new feature with these minimum versions
This will help users understand the version requirements when using the labelPlacement feature.
Also applies to: 46-47, 40-41
🛑 Comments failed to post (2)
plop/component/package.json.hbs (1)
39-41: 💡 Codebase verification
Both theme and system packages should be aligned to their latest versions
The verification reveals that:
- The system package (2.4.4) is actually ahead of the theme package (2.4.3)
- The system package has labelPlacement-related files, indicating it's actively involved in this feature
- The template should use the latest available versions for both packages
Update the peer dependencies in the template to:
"@nextui-org/theme": ">=2.4.3""@nextui-org/system": ">=2.4.4"🔗 Analysis chain
Verify version alignment between theme and system packages
The theme package is bumped to
>=2.4.3while system remains at>=2.4.0. Since this PR adds global labelPlacement support, please verify if @nextui-org/system should also be bumped to maintain version alignment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if system package has related changes or dependencies on theme package # Search for labelPlacement-related changes in system package rg -l "labelPlacement" packages/core/system/ # Check package.json files for current versions fd -g "package.json" packages/core/system/ -x cat {} | jq -r 'select(.name=="@nextui-org/system") | .version' fd -g "package.json" packages/core/theme/ -x cat {} | jq -r 'select(.name=="@nextui-org/theme") | .version'Length of output: 415
packages/components/date-picker/package.json (1)
37-38: 💡 Codebase verification
Update required: Dependency version constraint needs adjustment
The specified version
@nextui-org/[email protected]is not yet published (latest available is 2.4.4). While other components consistently use>=2.4.5, this version is not yet available on npm. The theme dependency version>=2.4.3is valid as this version exists.
- Update
@nextui-org/systemto use>=2.4.4instead of>=2.4.5until the new version is published- Keep
@nextui-org/themeat>=2.4.3as it's valid🔗 Analysis chain
Verify dependency version compatibility
The peer dependency updates look consistent with the global labelPlacement feature implementation. Let's verify the version compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the specified versions exist and are stable # Check if the required versions are published on npm echo "Checking @nextui-org/system version..." npm view @nextui-org/system versions --json | jq 'contains(["2.4.5"])' echo "Checking @nextui-org/theme version..." npm view @nextui-org/theme versions --json | jq 'contains(["2.4.3"])' # Check for any deprecation notices npm view @nextui-org/system deprecated npm view @nextui-org/theme deprecatedLength of output: 408
Script:
#!/bin/bash # Let's check the latest available versions and version ranges echo "Latest @nextui-org/system versions:" npm view @nextui-org/system versions --json | jq -r '.[-5:]' # Check other components' package.json files for version consistency echo -e "\nChecking other components' dependencies:" fd package.json --type f --exec grep -l "@nextui-org/system" {} \; | xargs cat | grep "@nextui-org/system"Length of output: 4000
|
I made the changes to maintain uniformity among the package's peerDependency. |
macci001
force-pushed
the
macci001/eng-1694
branch
from
1a0a128 to
03e9405
Compare
wingkwong
changed the title
Closes #
📝 Description
Adds the support for the labelPlacement prop in NextUI provider.
🚀 New behavior
💣 Is this a breaking change (Yes/No): No
Summary by CodeRabbit
Release Notes
New Features
labelPlacementproperty for various components, enhancing label positioning flexibility.labelPlacementprop toNextUIProvider, allowing specification of label positions.Bug Fixes
Documentation
labelPlacementprop details.Chores