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.

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

feat: add datetime components with hook form integration #42

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

walt-it
Copy link
Contributor

@walt-it walt-it commented Oct 21, 2024

Add date time components

Currently we don't have any standard for date or time components, mostly because the libraries we usually reach out for (Radix and Headless) do not have them. This results in each project making different decisions for which one to use or in some cases even writing these complex components from scratch which is very error prone, leaving out accessibility concerns or not contemplating a lot of edge cases.

This proposal aims to add an implementation for a batch of components related to date and time selection, using these components from react-aria as a solid base.

Screenshot 2024-11-13 at 12 13 21
Screenshot 2024-11-13 at 12 13 32

Also includes a proposal for making these components usable with and without hook form integration. Special thanks to @slagomarsino-dev for the Typescript help on this!

If you want to try out the components, you can pull the branch and go to the /playground route. (I'll delete this before merging if we move forward and move the examples over to Storybook)

@github-actions github-actions bot added the WIP Work in progress or draft label Oct 21, 2024
@walt-it walt-it changed the title No task/add datetime components with hook form integration feat: add datetime components with hook form integration Oct 21, 2024
@walt-it walt-it requested a review from ThunderNaka November 13, 2024 14:57
@walt-it walt-it removed the WIP Work in progress or draft label Nov 13, 2024
@hyanez-lightit
Copy link
Contributor

Can we add some stories for it ?

Copy link
Contributor

@slagomarsino-dev slagomarsino-dev left a comment

Choose a reason for hiding this comment

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

Awesome work, you nailed it! 🚀 🔥

src/domains/guest/screens/Playground.tsx Outdated Show resolved Hide resolved
src/domains/guest/screens/Playground.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/Calendar/Calendar.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/Calendar/CalendarHeader.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/Calendar/CalendarHeader.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/DateField/DateField.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/DatePicker/DatePicker.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/DatePicker/DatePicker.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/RangeCalendar/RangeCalendar.tsx Outdated Show resolved Hide resolved
src/shared/components/ui/RangeCalendar/RangeCalendar.tsx Outdated Show resolved Hide resolved
@@ -3,3 +3,5 @@ export const dateWithoutTimezone = (input: string) => {
const withoutTimezone = date.substring(0, date.length - 1);
return new Date(withoutTimezone);
};

export const currentTimezone = Intl.DateTimeFormat().resolvedOptions().timeZone;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird colocation for this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't talked about this yet, but I think we should figure out some sort of classification for utils. I feel like the way it's setup right now we will end up with too many files that only have one fn inside.

I can move it to its own file in the meantime.

Comment on lines +1 to +8
export const findFirstEnabledDate = (
table: HTMLTableElement | null,
): Element | null => {
return (
table?.querySelector<HTMLTableCellElement>("td:not([aria-disabled])")
?.children[0] ?? null
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necesary? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to choose a place to put the controller.field.ref in order for hook form to focus on the correct element when the field is invalid. For calendars I chose the first enabled date and for that I need to reach into the table and choose the correct component to focus.

There's a number of ways we could do this, another option could be to focus on a wrapper div and use focus-within from tw to style it. But that also brings accessibility concerns because we have to manage screen reader labels ourselves.

I think pointing to the element that react-aria already manages it's the best choice.

@@ -0,0 +1,2 @@
export const isDateISOString = (dateString: string) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not have this function in Light-it yet, is it mandatory to have this?

https://github.com/search?q=org%3ALight-it-labs+isDateISOString&type=code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in the validations file in this PR to generate zod rules for both a date and a range of dates.
I think it would be a nice addition to the boilerplate to have a standard on date format and ISO seems like a good choice.

Comment on lines +22 to +28
const classNames = tv({
slots: {
dateInput:
"flex w-full items-center space-x-1 rounded-lg border p-2 md:w-fit",
dateSegment: "font-medium text-gray-800",
},
})();
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are not going by the standard executing the tv function inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a suggestion by @slagomarsino-dev to group the styles into a classNames namespace and avoid confusion in the template because in this case names like dateInput or dateSegment might be confused with something else.

I agree with that but I'm also ok with calling them in the template, I think using them inside the classNames prop usually it's a good enough indication that it will output a class.

Comment on lines 8 to 11
default: "",
selected: "bg-purple-500 text-white",
disabled: "cursor-not-allowed bg-slate-500 text-slate-400",
unavailable: "cursor-not-allowed bg-red-200 text-red-300",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, you can just have a selected, disabled, and unavailable variant instead of a "state." Then, you can use them.

})();

interface TimeFieldBaseProps<T extends TimeValue>
extends Omit<AriaTimeFieldProps<T>, "className"> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we omitting className?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was based on a couple of things:

  • The className prop needs to be passed to an element inside, I'm assuming best place would be the wrapper element to use the override to add external margins for example. In that case, each project can make the decision to add this prop only when it's needed.
  • The className prop coming from react-aria can be both a string or a fn that receives component state (isDisabled for example) so it would require some additional checks to use it
  • I think it would be best to treat the ui components style as internal and manage any external spaces through other means, like flexbox or grid containers

Comment on lines +45 to +47
label,
description,
errorMessage,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

<DateInput className={classNames.dateInput}>
{(segment) => (
<DateSegment
ref={(el) => isFirstChild(el) && controller?.field.ref(el)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Comment on lines +1 to +6
export const isFirstChild = (el: Element | null) => {
if (!el) return false;
return (
el.parentElement && Array.from(el.parentElement.children).indexOf(el) === 0
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the point of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as this: #42 (comment)

But in this case choosing the first date segment from the field to focus when it's invalid.

Comment on lines +6 to +14
export interface WithHookForm<U extends FieldValues> {
name: Path<U>;
control: Control<U>;
onChange?: never;
onBlur?: never;
}
// This type allows onChange and onBlur to be passed when there is no control.
// Also makes name optional and a generic string instead of a form Path.
export interface WithoutHookForm<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Semantics, but both have hook forms; the difference is when they are controlled or uncontrolled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand this, if I don't pass control to the component then hook form should have no idea that particular field exists. I am using useController inside on either case, is that what you meant?

These names makes sense to me but I'm open to change them to whatever seems clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants