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(Calendar): implement component #2618

Open
wants to merge 72 commits into
base: v3
Choose a base branch
from
Open

Conversation

hywax
Copy link

@hywax hywax commented Nov 12, 2024

πŸ”— Linked issue

Resolves #1137 and half #2524

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

UDatepicker component to use nuxt-ui is sorely lacking. This is the first step towards implementing a UDatepicker. I'll be doing this in my spare time, so it might be a bit of a stretch.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@benjamincanac benjamincanac changed the title feat(calendar): implement component feat(Calendar): implement component Nov 12, 2024
@benjamincanac benjamincanac added the v3 #1289 label Nov 12, 2024
@hywax
Copy link
Author

hywax commented Nov 13, 2024

@benjamincanac do you think UCalendar is part of UForm? Or is it an independent component that only becomes part of UDatepicker?

I'm asking to see if I need to get the props part from useFormField

Upd. looked at how other ui's work with the calendar. It's always a separate component, without a form, it's with a model

Copy link
Member

No need to make it work in a form, it's a separate component indeed! Also, it should handle ranges:

src/theme/calendar.ts Outdated Show resolved Hide resolved
@hywax
Copy link
Author

hywax commented Nov 13, 2024

@benjamincanac at the moment it looks like this. It remains to solve the typing problem and write documentation.

Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

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

You should not duplicate the code to handle range prop but use namespaced imports instead: https://www.radix-vue.com/guides/namespaced-components.html

Take a look at https://github.com/nuxt/ui/blob/v3/src/runtime/components/DropdownMenuContent.vue#L92 for example.

You could have something like const Component = computed(() => props.range ? RangeCalendar : Calendar).

@hywax
Copy link
Author

hywax commented Nov 13, 2024

I was going to do that originally. But I thought that typing on dynamic components might break.

But since you recommend it, I'll do it like this

@hywax
Copy link
Author

hywax commented Nov 14, 2024

@benjamincanac radix uses @internationalized/date for calendars, which internally creates a class. When Vue wraps this class in ref, it hides some of the private properties. The solution to this situation is to use shallowRef.


There is an issue where this problem was discussed: vuejs/core#2981 (comment), but the workaround looks quite unappealing.

I would suggest switching to standard new Date in the models.

Example:

export type DateRange = {
-    start: DateValue | undefined;
-    end: DateValue | undefined;
+    start: Date | undefined;
+    end: Date | undefined;
};

What do you think about this?

PS Even in the example they use computed in the model, not ref because the model is computed inside the class - https://www.radix-vue.com/components/calendar.html#calendar-with-locale-and-calendar-system-selection

@hywax
Copy link
Author

hywax commented Nov 21, 2024

@benjamincanac done. Also returned the flex styles for body, needed for 2 or more calendars

@hywax
Copy link
Author

hywax commented Nov 21, 2024


@hywax
Copy link
Author

hywax commented Nov 21, 2024

@benjamincanac I switched from the standard new Date to using @internationalized/date. I think this approach is more appropriate.

Pros:

  • It reduced the amount of code
  • All props/events can be used

Cons:

@hywax
Copy link
Author

hywax commented Nov 21, 2024

I don't plan to add or modify the code anymore. You can review it, and if everything looks good, go ahead and merge it 😊

Copy link
Member

Awesome! Will try to make a final review tomorrow and merge it :) In both datepickers examples, would you mind formatting the dates as such: Jan 20, 2022 - Feb 9, 2022? 😬

@hywax
Copy link
Author

hywax commented Nov 22, 2024

Done, additionally added some usage examples

ui?: Partial<typeof calendar.slots>
}
export interface CalendarEmits extends Omit<CalendarRootEmits & RangeCalendarRootEmits, 'update:modelValue'> {}
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the Omit here so the docs show the update:modelValue event.

Copy link
Author

Choose a reason for hiding this comment

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

In the documentation, update:modelValue is already displayed, it is taken from defineModel.

Comment on lines +79 to +81
const parsedProps = useForwardProps(reactiveOmit(props, 'color', 'size', 'range', 'monthControls', 'yearControls', 'class', 'ui', 'modelValue' as any))
const emitsAsProps = objectOmit(useEmitAsProps(emits), ['update:modelValue'])
const rootProps = computed(() => ({ ...parsedProps.value, ...emitsAsProps }))
Copy link
Member

Choose a reason for hiding this comment

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

Why not use useForwardPropsEmits?

Copy link
Author

Choose a reason for hiding this comment

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

Initially, I used useForwardPropsEmits, but it provides update:modelValue, which in turn duplicates the update of v-model. I fixed it in ee422ee.

Simply removing modelValue doesn't work because defineModel creates the update:modelValue event, which in turn is not removed by useForwardPropsEmits.

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

Successfully merging this pull request may close these issues.

2 participants