fix(date-range-picker): fixed input overflow #3076
Conversation
🦋 Changeset detectedLatest commit: d191b82 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 ↗︎
|
|
Someone is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update primarily addresses layout issues with the Changes
Assessment against linked issues
Possibly related PRs
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: 0
Outside diff range and nitpick comments (3)
packages/components/date-picker/src/use-date-range-picker.ts (3)
Line range hint
98-98: Consider usingconstinstead ofletfor variables that are assigned only once.- let state: DateRangePickerState = useDateRangePickerState({ + const state: DateRangePickerState = useDateRangePickerState({Also applies to: 106-106
Line range hint
123-123: Simplify the dependency arrays for theuseMemohooks.- [objectToDeps(variantProps), className], + [variantProps, className],Also applies to: 267-267
Line range hint
214-214: Simplify the computed expression without using a string literal.- label: stringFormatter.format("startTime"), + label: stringFormatter.format(startTime),
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/components/date-picker/src/use-date-range-picker.ts (1 hunks)
- packages/core/theme/src/components/date-input.ts (2 hunks)
Additional Context Used
Biome (8)
packages/components/date-picker/src/use-date-range-picker.ts (8)
214-214: The computed expression can be simplified without the use of a string literal.
98-98: This let declares a variable that is only assigned once.
106-106: This let declares some variables that are only assigned once.
123-123: This hook does not specify all of its dependencies: variantProps
123-123: This hook specifies more dependencies than necessary: objectToDeps(variantProps)
267-267: This hook does not specify all of its dependencies: labelPlacement
267-267: This hook does not specify all of its dependencies: variantProps
267-267: This hook specifies more dependencies than necessary: objectToDeps(variantProps)
Additional comments not posted (3)
packages/core/theme/src/components/date-input.ts (2)
12-12: LGTM! The addition ofoverflow-hiddento thebaseslot should help prevent overflow issues.
24-24: LGTM! The addition ofoverflow-hiddento theinputslot should help manage content overflow effectively.packages/components/date-picker/src/use-date-range-picker.ts (1)
293-293: LGTM! SettingflexShrink: 0ensures that the date input fields do not shrink, helping to maintain layout consistency on smaller screens.
| @@ -290,6 +290,7 @@ export function useDateRangePicker<T extends DateValue>({ | |||
| style: { | |||
| ...props.style, | |||
| maxWidth: "fit-content", | |||
| flexShrink: 0, | |||
There was a problem hiding this comment.
do we have to set it here instead of packages/core/theme/src/components/date-input.ts?
There was a problem hiding this comment.
I have set it here as there was no slot just for start-input.
There was a problem hiding this comment.
Regarding the scrolling, could we wrap the start-input and end-input inside a div? So that we could achieve something like this
There was a problem hiding this comment.
I have set it here as there was no slot just for start-input.
just a few lines below
className: dateInputSlots.input({
class: clsx(classNames?.input, props?.className),
}),Regarding the scrolling, could we wrap the start-input and end-input inside a div? So that we could achieve something like this
I think it's fine. We should only scroll the input. The scrollbar can look better tho.
There was a problem hiding this comment.
just a few lines below
className: dateInputSlots.input({ class: clsx(classNames?.input, props?.className), }),
I guess we might not need flexShrink: 0 as we are adding a scrollbar.
I think it's fine. We should only scroll the input.
If we dont add a div wrapper then the complete input-wrapper will be scrolled and by default the calendar button is not visible, we need to scroll to press the calendar button.
The scrollbar can look better tho.
I have used the default scrollbar like the components Table or ScrollShadow.
Is their a custom scrollbar already present or do you want to me implement one?
Apart from the default scrollbar, the only scrollbar i found being used in the project is the below one? Do you want to me make use of this?
There was a problem hiding this comment.
use the default scrollbar first. we may refactor later.
There was a problem hiding this comment.
I guess we might need to increase the height of the component or customize the height of the scrollbar as just applying scrollbar hides the input as shown below:-
Do you want me to increase the height? If yes could you tell me the values to which i need to increase?
Currently the innerWrapper height is set h-6 and the size of the inputWrapper is h-8 h-10 h-12 for size sm md & lg respectively.
Or should we create a custom scrollbar so that it could be used in other places and consistency is maintained?
There was a problem hiding this comment.
i think not. the height shouldn't be changed.
There was a problem hiding this comment.
If the height is not changed then the text will not be visible.
There was a problem hiding this comment.
the scrollbar shouldn't look like this. may double check with other pages. I rmb it was quite thin.
wingkwong
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/components/date-picker/src/use-date-range-picker.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/components/date-picker/src/use-date-range-picker.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- .changeset/ten-ways-confess.md (1 hunks)
Additional comments not posted (1)
.changeset/ten-ways-confess.md (1)
1-5: Changeset Documentation Looks Good!The changeset correctly identifies the package (
@nextui-org/date-picker) and the nature of the change (patch). The description is clear and directly addresses the issue fixed by this patch.
| @@ -294,6 +294,7 @@ export function useDateRangePicker<T extends DateValue>({ | |||
| style: { | |||
| ...props.style, | |||
| maxWidth: "fit-content", | |||
| flexShrink: 0, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more comprehensive solution for the overflow issue
While adding flexShrink: 0 may prevent the start date input from shrinking, it might not fully address the overflow problem described in the PR objectives. This solution could potentially create layout issues in certain scenarios, especially on smaller screens.
Instead, consider implementing a more robust solution that addresses the root cause of the overflow:
- Wrap both start and end inputs in a scrollable container, as suggested in the past review comments.
- Implement text truncation for long date strings, as mentioned in the expected behavior from the linked issue.
- Ensure the calendar button remains visible and accessible at all times.
Here's a potential implementation:
-style: {
- ...props.style,
- maxWidth: "fit-content",
- flexShrink: 0,
-},
+style: {
+ ...props.style,
+ maxWidth: "100%",
+ overflow: "hidden",
+ textOverflow: "ellipsis",
+ whiteSpace: "nowrap",
+},
+wrapperStyle: {
+ display: "flex",
+ overflow: "auto",
+ maxWidth: "100%",
+},Then, in the component that uses these props, wrap the inputs in a div with the wrapperStyle.
This approach would allow the date text to truncate when necessary, while still providing a scrollable interface for users to see the full date if needed.
Would you like assistance in implementing this solution across the component?
Committable suggestion was skipped due to low confidence.
Closes #3037
📝 Description
When the component was on mobile screen, its contents were overflowing.
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
If the PR gets accepted please use my GitHub email-id (shrinidhiupadhyaya1195@gmail.com) instead of my other email-id for the Co-authored-by: message.
Summary by CodeRabbit
New Features
classNamesproperty for enhanced customization of date range picker components.Style
flexShrink: 0.overflow-hiddento prevent content overflow.Bug Fixes