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

Calendar's controlled value prop (in onChange) and setValue() from CalendarStateContext can't be set to null #6006

Closed
psywalker opened this issue Mar 5, 2024 · 7 comments · Fixed by #6030
Labels
bug Something isn't working good first issue Good for newcomers RAC rsp:Calendar

Comments

@psywalker
Copy link
Contributor

psywalker commented Mar 5, 2024

Provide a general summary of the issue here

I get the setValue method from CalendarStateContext and try to set null as the value for setValue() but an error immediately occurs and nothing is set. Prop of value and onChange and setValue(null) with the wrong type and TS swears at null.

🤔 Expected Behavior?

Calendar's controlled value prop and state.setValue() from CalendarStateContext and in onChange prop can be set to null

😯 Current Behavior

Calendar controlled value and setValue() from CalendarStateContext can't be set to null, judging by value?:T, but for some reason it allows you to set null.
And the same goes for method setValue() from CalendarStateContext. You cannot set null in setValue()

💁 Possible Solution

  1. Add to the interface ValueBase<T, C = T>:

value?: T | null,
onChange?: (value: C (and null))

export interface ValueBase<T, C = T> {
  /** The current value (controlled). */
  value?: T,
  /** The default value (uncontrolled). */
  default Value?: T,
  /** Handler that is called when the value changes. */
  onChange?: (value: C) => void
}
  1. Add to the interface CalendarState extends CalendarStateBase:

setValue(value: CalendarDate | null): void;

export interface CalendarState extends CalendarStateBase {
    /** The currently selected date. */
    readonly value: CalendarDate;
    /** Sets the currently selected date. */
    setValue(value: CalendarDate): void;
}
  1. Give the option (fix) to set null so that you can, for example, reset the date in the calendar.

🔦 Context

No response

🖥️ Steps to Reproduce

Here you can see an example. You can see that onChange and setValue(null) with the wrong type and TS swears at null. And if you press the "Clear" button and try to set state.setValue(null); and the value in value is not reset

Version

1.1.1 react-aria-components

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

MacOS 13.3

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@LFDanLu
Copy link
Member

LFDanLu commented Mar 6, 2024

Looks like the same issue as mentioned here: #3187 (comment). I believe that we can just add a if statement before here:

newValue = constrainValue(newValue, minValue, maxValue);

to be something like

      if (newValue === null) {
        setControlledValue(null);
        return;
      }

Mind opening a PR?

@LFDanLu LFDanLu added good first issue Good for newcomers rsp:Calendar RAC bug Something isn't working labels Mar 6, 2024
@psywalker
Copy link
Contributor Author

psywalker commented Mar 7, 2024

Looks like the same issue as mentioned here: #3187 (comment). I believe that we can just add a if statement before here:

newValue = constrainValue(newValue, minValue, maxValue);

to be something like

      if (newValue === null) {
        setControlledValue(null);
        return;
      }

Mind opening a PR?

OK, I'll try to do this task.

@psywalker
Copy link
Contributor Author

@LFDanLu Please help me understand in which interfaces to add ... | null

@LFDanLu
Copy link
Member

LFDanLu commented Mar 7, 2024

I imagine

setValue(value: CalendarDate): void
would be an area to add the | null to.

@psywalker
Copy link
Contributor Author

psywalker commented Mar 9, 2024

@LFDanLu, done! #6030

UPD:
Sorry, I noticed that the linter fell down. I'm checking, and there's this

[0] - Compiling with strict mode...
[0] packages/react-aria-components/stories/Calendar.stories.tsx(22,10): error TS2339: Property 'setValue' does not exist on type 'CalendarState | null'.

And also, when I hover over setValue, it gives me this:
const setValue: (value: CalendarDate) => void

Although in fact, this is what the code looks like:

export interface CalendarState extends CalendarStateBase {
  /** The currently selected date. */
  readonly value: CalendarDate | null,
  /** Sets the currently selected date. */
  setValue(value: CalendarDate | null): void
}

It seems like it's just not visible | null

And we can also see that my fix is working. This can be viewed in the "Test Instructions" section of my PR. Even on the video that I attached to my PR, it is also clearly visible.

@LFDanLu
Copy link
Member

LFDanLu commented Mar 11, 2024

Oh odd, I'll see if I can take a look soon!

@psywalker
Copy link
Contributor Author

@LFDanLu Well, thank you! I've put my PR in the draft for now, just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers RAC rsp:Calendar
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants