Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(date-range-picker): fixed input overflow #3076

Open
wants to merge 5 commits into
base: canary
Choose a base branch
from
Open

fix(date-range-picker): fixed input overflow #3076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8dbb37a
fix(date-range-picker): fixed input overflow #3037
May 25, 2024
868d60a
Merge branch 'nextui-org:canary' into fix/date-range-picker
ShrinidhiUpadhyaya Jun 27, 2024
cbd4c38
fix(date-range-picker): included changeset
Jun 27, 2024
8303698
Merge branch 'nextui-org:canary' into fix/date-range-picker
ShrinidhiUpadhyaya Jul 3, 2024
d191b82
Merge branch 'nextui-org:canary' into fix/date-range-picker
ShrinidhiUpadhyaya Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/ten-ways-confess.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@nextui-org/date-picker": patch
---

Fix input overflow in date-range-picker
1 change: 1 addition & 0 deletions packages/components/date-picker/src/use-date-range-picker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ export function useDateRangePicker<T extends DateValue>({
style: {
...props.style,
maxWidth: "fit-content",
flexShrink: 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have to set it here instead of packages/core/theme/src/components/date-input.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have set it here as there was no slot just for start-input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the scrolling, could we wrap the start-input and end-input inside a div? So that we could achieve something like this

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

image

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?
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the default scrollbar first. we may refactor later.

Copy link
Contributor Author

@ShrinidhiUpadhyaya ShrinidhiUpadhyaya Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:-
image

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think not. the height shouldn't be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the height is not changed then the text will not be visible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the scrollbar shouldn't look like this. may double check with other pages. I rmb it was quite thin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Wrap both start and end inputs in a scrollable container, as suggested in the past review comments.
  2. Implement text truncation for long date strings, as mentioned in the expected behavior from the linked issue.
  3. 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.

},
className: dateInputSlots.input({
class: clsx(classNames?.input, props?.className),
Expand Down
4 changes: 2 additions & 2 deletions packages/core/theme/src/components/date-input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {tv} from "../utils/tv";
*/
const dateInput = tv({
slots: {
base: "group flex flex-col",
base: "group flex flex-col overflow-hidden",
label: [
"block subpixel-antialiased text-small text-default-600",
// isRequired=true
Expand All @@ -21,7 +21,7 @@ const dateInput = tv({
"relative px-3 gap-3 w-full inline-flex flex-row items-center",
"cursor-text tap-highlight-transparent shadow-sm",
],
input: "flex h-full gap-x-0.5 w-full font-normal",
input: "flex h-full gap-x-0.5 w-full font-normal overflow-hidden",
innerWrapper: [
"flex items-center text-default-400 w-full gap-x-2 h-6",
// isInValid=true
Expand Down