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: startValue binding for RangeCalendar/DateRangePicker #824

Merged
merged 17 commits into from
Apr 11, 2024

Conversation

epr3
Copy link
Collaborator

@epr3 epr3 commented Apr 5, 2024

This PR adds a new convenience property to RangeCalendar/DateRangePicker for setting up the startValue on the aforementioned components plus an event for listening for said startValue changes. The property cannot be modified outside the component, you would need to update the modelValue for that.

Also, this PR cleans up some of the conditions inside the DateRange components for consistent behaviour regarding modelValue updates plus some clean code updates on the defaultValue.

Context for feature: https://www.shadcn-svelte.com/docs/components/date-picker#date-range-picker

cc @sadeghbarati for the feature suggestion

@epr3 epr3 changed the title Feature: startValue binding for RangeCalendar/DateRangePicker feat: startValue binding for RangeCalendar/DateRangePicker Apr 5, 2024
Copy link
Collaborator

@zernonia zernonia left a comment

Choose a reason for hiding this comment

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

Let's also add a story in Chromatic to showcase this ya 😁

@epr3
Copy link
Collaborator Author

epr3 commented Apr 7, 2024

@zernonia I've updated the watcher inner variables in both DateRangeField and RangeCalendar and added the startValue story in RangeCalendarChromatic.story.vue.

@@ -47,8 +49,16 @@ const placeholder = ref(new CalendarDate(2024, 4, 1)) as Ref<CalendarDate>
<RangeCalendar :default-value="defaultValue" prevent-deselect />
</Variant>

<Variant title="Start value">
<RangeCalendar :default-value="defaultValue" :start-value="startValue" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

removing defaultValue, and allow startValue to take action, causing the placeholder month to be different from the startValue.

Maybe let's show the same month as the startValue? That behave more "natural"

image

@zernonia
Copy link
Collaborator

zernonia commented Apr 8, 2024

Also, what's the benefit of adding this new prop ya? Can't we use modelValue with value { start: new CalendarDate(2024, 2, 20), end: undefined } to achieve the same?

And I don't quite understand you mentioned

Context for feature: https://www.shadcn-svelte.com/docs/components/date-picker#date-range-picker

Is the startValue being used anywhere?

@epr3
Copy link
Collaborator Author

epr3 commented Apr 8, 2024

The prop can be used for getting a fresh startValue whenever the user first clicks a date range, since the RangeCalendar only updates the modelValue once both the start and the end are filled.

On the shadcn-svelte link, the startValue is used for this piece of code(the DateRangePicker)(I've marked with comments where the startValue is used):

<script lang="ts">
  import CalendarIcon from "lucide-svelte/icons/calendar";
  import type { DateRange } from "bits-ui";
  import {
    CalendarDate,
    DateFormatter,
    type DateValue,
    getLocalTimeZone
  } from "@internationalized/date";
  import { cn } from "$lib/utils.js";
  import { Button } from "$lib/components/ui/button/index.js";
  import { RangeCalendar } from "$lib/components/ui/range-calendar/index.js";
  import * as Popover from "$lib/components/ui/popover/index.js";
 
  const df = new DateFormatter("en-US", {
    dateStyle: "medium"
  });
 
  let value: DateRange | undefined = {
    start: new CalendarDate(2022, 1, 20),
    end: new CalendarDate(2022, 1, 20).add({ days: 20 })
  };
 
  let startValue: DateValue | undefined = undefined; // here
</script>
 
<div class="grid gap-2">
  <Popover.Root openFocus>
    <Popover.Trigger asChild let:builder>
      <Button
        variant="outline"
        class={cn(
          "w-[300px] justify-start text-left font-normal",
          !value && "text-muted-foreground"
        )}
        builders={[builder]}
      >
        <CalendarIcon class="mr-2 h-4 w-4" />
        {#if value && value.start}
          {#if value.end}
            {df.format(value.start.toDate(getLocalTimeZone()))} - {df.format(
              value.end.toDate(getLocalTimeZone())
            )}
          {:else}
            {df.format(value.start.toDate(getLocalTimeZone()))}
          {/if}
        {:else if startValue} // here
          {df.format(startValue.toDate(getLocalTimeZone()))}
        {:else}
          Pick a date
        {/if}
      </Button>
    </Popover.Trigger>
    <Popover.Content class="w-auto p-0" align="start">
      <RangeCalendar
        bind:value
        bind:startValue //here
        initialFocus
        numberOfMonths={2}
        placeholder={value?.start}
      />
    </Popover.Content>
  </Popover.Root>
</div>

@epr3
Copy link
Collaborator Author

epr3 commented Apr 9, 2024

@zernonia I gave more of a thought about the startValue prop: you're right, we don't actually need it, but the @update:start-value event is useful for always getting a fresh startValue from the RangeCalendar.

I've also renamed the inner watcher variables as suggested. 🚀

@zernonia
Copy link
Collaborator

zernonia commented Apr 10, 2024

@zernonia I gave more of a thought about the startValue prop: you're right, we don't actually need it, but the @update:start-value event is useful for always getting a fresh startValue from the RangeCalendar.

I've also renamed the inner watcher variables as suggested. 🚀

Icic.. How about we only expose update:startValue emit? and user can use this emit to manipulate the modelValue.

@epr3
Copy link
Collaborator Author

epr3 commented Apr 10, 2024

@zernonia I gave more of a thought about the startValue prop: you're right, we don't actually need it, but the @update:start-value event is useful for always getting a fresh startValue from the RangeCalendar.
I've also renamed the inner watcher variables as suggested. 🚀

Icic.. How about we only expose update:startValue emit? and user can use this emit to manipulate the modelValue.

Sure thing! I've exposed the event and removed the startValue property in the latest commits. 👍🏻

@zernonia zernonia merged commit 4a2b331 into radix-vue:main Apr 11, 2024
2 of 3 checks passed
@epr3 epr3 deleted the fix-range-calendar-emit branch April 11, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants