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

DateRangePicker's controlled value can't be set to null to show the placeholder instead of the previous selected date #3187

Closed
AlexLengyel opened this issue Jun 1, 2022 · 15 comments · Fixed by #4312
Assignees
Labels
bug Something isn't working

Comments

@AlexLengyel
Copy link

🐛 Bug Report

Value prop can't be set to null (as in the documentation) in DateRangePicker because TypeScript needs a DateValue type of value. Once the value prop has received a value then the value can't be removed completely from the date field to show just a placeholder again.

🤔 Expected Behavior

If the value prop in the DateRangePicker receives a null or undefined value then the previous selected date should be removed from the date field and should display a placeholder again instead of the previous selected date.

😯 Current Behavior

If the value prop in the DateRangePicker receives a null or undefined value then the previous selected date stays there in the date field.

🔦 Context

I am trying to reset the DateRangePicker's value to nothing to show the placeholder again after the previously selected date has been removed from the filtering list, so it makes less confusing for the users.

💻 Code Sample

🌍 Your Environment

Software Version(s)
react-spectrum 3.17.0
Browser Google Chrome
Operating System MacOS

🧢 Your Company/Team

AEM - Admin UI, Odin project

@dj-neza
Copy link

dj-neza commented Jul 12, 2022

Same issue here. The date range picker considers undefined value as a sign of an uncontrolled component and what's even weirder, seems to revert selection to the first selection ever made (e.g. if the user tries a few different options before attempting to reset the range altogether).

Haven't tested a lot and setting the value to { start: null, end: null } looks ok but in that case Typescript becomes a problem since start and end are not allowed to be null. Ideally that should be possible to enable a 'truly' controlled component.

@mrcljx
Copy link

mrcljx commented Oct 28, 2022

I guess ValueBase needs to accept null for forcing controlled mode?

  export interface ValueBase<T, C = T> {
    /** The current value (controlled). */
-   value?: T,
+   value?: T | null,
    /** The default value (uncontrolled). */
    defaultValue?: T,
    /** Handler that is called when the value changes. */
    onChange?: (value: C) => void
}

@lishichengyan
Copy link
Contributor

I'm trying to repro the issue with the latest version, here's the code sample -

function ControlledResetExample(props) {
  let [value, setValue] = React.useState(null);

  return (
    <Flex direction="column" alignItems="center" gap="size-150">
      <DateRangePicker label="Controlled" {...props} value={value} onChange={chain(setValue, action('onChange'))} />
      <ActionButton onPress={() => setValue({start: new CalendarDate(2020, 2, 3), end: new CalendarDate(2020, 5, 4)})}>Change value</ActionButton>
      <ActionButton onPress={() => setValue(null)}>Clear</ActionButton>
    </Flex>
  );
}

Seems to me setting value to null works just fine.

@snowystinger
Copy link
Member

@lishichengyan do you have TS strict mode on? that can mess up some of these explicit vs implicit null's

@lishichengyan
Copy link
Contributor

@lishichengyan do you have TS strict mode on? that can mess up some of these explicit vs implicit null's

no, in tsconfig.json I see "strict": false,

@B0und
Copy link

B0und commented Nov 7, 2022

I'm trying to repro the issue with the latest version, here's the code sample -

function ControlledResetExample(props) {
  let [value, setValue] = React.useState(null);

  return (
    <Flex direction="column" alignItems="center" gap="size-150">
      <DateRangePicker label="Controlled" {...props} value={value} onChange={chain(setValue, action('onChange'))} />
      <ActionButton onPress={() => setValue({start: new CalendarDate(2020, 2, 3), end: new CalendarDate(2020, 5, 4)})}>Change value</ActionButton>
      <ActionButton onPress={() => setValue(null)}>Clear</ActionButton>
    </Flex>
  );
}

Seems to me setting value to null works just fine.

This issue appears to be related to granularity. If you set it to minute then the behaviour becomes weird. I made a repro:

https://codesandbox.io/s/loving-christian-ciisig?file=/src/DateRangePicker.js

  1. Press on the calendar and select a date range

  2. close it and press reset button

  3. if you open the datepicker again, old values are persisted (i would expect them to also get reset)

P.S also calling setValue(null) - errors. (uncomment it in the file to repro)

@B0und
Copy link

B0und commented Nov 18, 2022

@lishichengyan if anything is unclear, please let me know

@Pagebakers
Copy link

Running into this as well, also without granularity set to minute.

When setting the value to null, it doesn't reset the value and clear the segments.
Your example is helpful though, I suppose I need to explicitly call state.setValue to update the internal state, will try it out.

@LFDanLu
Copy link
Member

LFDanLu commented Dec 7, 2022

Apologies, I believe I misunderstood this issue the first time I looked at it, thought this was referring to a typescript strict mode issue. I've confirmed the issue where setting the date values to null isn't clearing the the visual range selected in the calendar dropdown when the granularity is set to "minute" or "second".

EDIT: Did some quick digging, looks like we properly update the tracked controlled value in useDateRangeState but not the date range value. I believe we can just call setSelectedDateRange and setSelectedTimeRange with null here at the very least, will need to move the state initialization up before that section

@LFDanLu LFDanLu added the bug Something isn't working label Dec 7, 2022
@LFDanLu LFDanLu moved this to ✏️ To Groom in RSP Component Milestones Dec 7, 2022
@LFDanLu LFDanLu removed the typescript label Jan 5, 2023
@matthewdeutsch matthewdeutsch moved this from ✏️ To Groom to 📋 Waiting for Sprint in RSP Component Milestones Jan 5, 2023
@dani-mp
Copy link

dani-mp commented Mar 7, 2023

I'm having the same issue with useDatePicker and TypeScript on strict mode. null is not allowed, and using undefined instead leaves the hook in an uncontrolled state, behaving differently.

Does anyone have any workaround?

@christofferok
Copy link

I'm having the same issue with useDatePicker and TypeScript on strict mode. null is not allowed, and using undefined instead leaves the hook in an uncontrolled state, behaving differently.

Does anyone have any workaround?

I ended up having to @ts-ignore those places to be able to use null 😨 Doesn't feel good, but hopefully just a temporary solution until things get fixed.

@dani-mp
Copy link

dani-mp commented Mar 8, 2023

@christofferok thanks, @christofferok - that's what I did too. 😞

@LFDanLu LFDanLu moved this from 📋 Waiting for Sprint to 🔬 To Investigate / Verify in RSP Component Milestones Mar 8, 2023
@LFDanLu LFDanLu moved this from 🔬 To Investigate / Verify to 📋 Waiting for Sprint in RSP Component Milestones Mar 8, 2023
@RIP21
Copy link

RIP21 commented Mar 16, 2023

As a workaround, I used pnpm patch functionality to patch the type for the time being.

diff --git a/src/inputs.d.ts b/src/inputs.d.ts
index 653b30af794558acb854a4fab593f514e8487d05..18e7ec3cf4d63537327f1f857c78a8f85ab16a02 100644
--- a/src/inputs.d.ts
+++ b/src/inputs.d.ts
@@ -33,7 +33,7 @@ export interface InputBase {

 export interface ValueBase<T, C = T> {
   /** The current value (controlled). */
-  value?: T,
+  value?: T | null,
   /** The default value (uncontrolled). */
   defaultValue?: T,
   /** Handler that is called when the value changes. */

You can use patch-package and other alternatives to achieve the same thing for other package managers.

@devongovett
Copy link
Member

The TypeScript strict mode part of this was addressed in #4225. There may be also a remaining behavioral issue mentioned by @LFDanLu above, which we will look at in an upcoming sprint.

@devongovett devongovett self-assigned this Mar 29, 2023
@devongovett devongovett moved this from 📋 Waiting for Sprint to 🏗 In Progress in RSP Component Milestones Mar 31, 2023
@devongovett devongovett moved this from 🏗 In Progress to 👀 In Review in RSP Component Milestones Mar 31, 2023
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in RSP Component Milestones Mar 31, 2023
@jake-chapman-mark43
Copy link

jake-chapman-mark43 commented Sep 7, 2023

@devongovett This works great for setting value outside of the component itself. But referencing state from inside of the component doesn't allow for setting null on any of the values.

const handleCalendarDateInputKeyDown = (
      e: React.KeyboardEvent,
      segment?: 'start' | 'end'
    ) => {
      const { key } = e;

      switch (key) {
        case 'Escape':
          state.setDateTime(segment, null as unknown as DateValue);
          return;
      }
    };

receives this error

Cannot destructure property 'hour' of '(intermediate value)' as it is undefined.

I've tried bunch of combinations. Closest I got was

const handleCalendarDateInputKeyDown = (
      e: React.KeyboardEvent,
      segment?: 'start' | 'end'
    ) => {
      const { key } = e;

      switch (key) {
        case 'Escape':
          const nullState = {
            year: null,
            month: null,
            day: null,
            hour: null,
            minute: null,
            second: null,
            millisecond: null,
          };
          if (segment === 'start') {
            state.setDateTime(segment, nullState as unknown as DateValue);
          }
          return;
      }
    };

But this gives

value.compare is not a function

Best way to reset value from internally using the state object returned by the hook?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.