-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
Expose DateInput
component
#988
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙏
Just a couple of minor doubts inline
const localTimeZone = getLocalTimeZone(); | ||
const internalProps = { | ||
...props, | ||
isDisabled: disabled, | ||
isReadOnly: readOnly, | ||
validationState: props.issues ? "invalid" : "valid", | ||
validationState: !props.isStandalone && props.issues ? "invalid" : "valid", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? Is there a specific reason to ignore the validationState
when used as a standalone component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it becausevalidationState
is relevant only for the field part, the standalone component doesn't use it
type Props = ( | |
| { type: "single"; fieldProps: AriaDatePickerProps<DateValue> } | |
| { | |
type: "range"; | |
fieldProps: { | |
start: AriaDatePickerProps<DateValue>; | |
end: AriaDatePickerProps<DateValue>; | |
}; | |
} | |
) & { | |
buttonProps: AriaButtonProps<"button">; | |
isCalendarOpen: boolean; | |
}; |
Should we also add it to the standalone component? It would be coherent with NumberInput
actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I see the internalProps
including it are passed only to useDatePicker
and useDatePickerState
which, according to the documentation, don't take a validationState
. So I think it's irrelevant 👍
</> | ||
); | ||
|
||
return props.isStandalone ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just expose the datePicker
as a DateInput
component and then define the DateField
as another component wrapping the DateInput?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first approach, but the useDatePicker
hook returns stuff that is necessary both for the input and the field. If we separate them into two components, wouldn't we have to use useDatePicker
both in DateInput
and DateField
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
No description provided.