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

TS Strict Calendar #6076

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

TS Strict Calendar #6076

wants to merge 10 commits into from

Conversation

snowystinger
Copy link
Member

Closes

I need help figuring out useRangeCalendarState I'm getting caught up trying to get the types to work for useControlledState

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@ishank-s
Copy link
Member

ishank-s commented Apr 9, 2024

hi can work on this if you 're not actively doing this.

@snowystinger
Copy link
Member Author

Thank you @ishank-s you're welcome to have a go at it

@ishank-s ishank-s mentioned this pull request Apr 14, 2024
5 tasks
@ishank-s
Copy link
Member

image

ishank-s and others added 2 commits May 13, 2024 18:35
# Conflicts:
#	packages/@react-stately/calendar/src/useCalendarState.ts
@rspbot
Copy link

rspbot commented May 13, 2024

@rspbot
Copy link

rspbot commented May 13, 2024

@rspbot
Copy link

rspbot commented May 21, 2024

@@ -71,7 +70,7 @@ export function useCalendarState<T extends DateValue = DateValue>(props: Calenda
} = props;
let calendar = useMemo(() => createCalendar(resolvedOptions.calendar), [createCalendar, resolvedOptions.calendar]);

let [value, setControlledValue] = useControlledState<DateValue>(props.value, props.defaultValue, props.onChange);
let [value, setControlledValue] = useControlledState<DateValue | null, MappedDateValue<T>>(props.value!, props.defaultValue ?? null!, props.onChange);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not certain we can fix useControlledState without overload syntax. This is the only way I've seen to prevent that syntax from propagating to every hook and component that uses useControlledState, but I'm open to other ideas.

I'm also unable to reproduce the issue in ts playground https://tinyurl.com/2p98znas
I don't know if that means I've over engineered it or have missed some setting

packages/@react-stately/calendar/src/useCalendarState.ts Outdated Show resolved Hide resolved
packages/@react-stately/calendar/src/utils.ts Outdated Show resolved Hide resolved
}

if (maxValue) {
date = minDate(date, toCalendarDate(maxValue));
let newDate = minDate(date, toCalendarDate(maxValue));
Copy link
Member

Choose a reason for hiding this comment

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

Why can minDate return null?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can't? only A | B | undefined

@@ -238,7 +238,7 @@ export function getWeeksInMonth(date: DateValue, locale: string): number {
}

/** Returns the lesser of the two provider dates. */
export function minDate<A extends DateValue, B extends DateValue>(a: A, b: B): A | B {
export function minDate<A extends DateValue, B extends DateValue>(a?: A, b?: B): A | B | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Where do we pass in null/undefined for these?

If we keep this, I guess the parameters shouldn't be optional but should allow null/undefined? I think making them optional only allows undefined not null.

Copy link
Member Author

Choose a reason for hiding this comment

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

let max = useMemo(() => minDate(maxValue, availableRange?.end), [maxValue, availableRange]);

Copy link
Member Author

Choose a reason for hiding this comment

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

why allow null? we only pass it undefined

Copy link
Member

Choose a reason for hiding this comment

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

Seems like if we are going to allow undefined we'd also want to allow null 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I think better not to have to account for both and we just restrict it to one of them. You can check it out in the latest commit though.

@rspbot
Copy link

rspbot commented May 31, 2024

@rspbot
Copy link

rspbot commented May 31, 2024

@rspbot
Copy link

rspbot commented May 31, 2024

@rspbot
Copy link

rspbot commented May 31, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
undefined already in set

@internationalized/date

minDate

 minDate<A extends DateValue, B extends DateValue> {
-  a: A
-  b: B
+  a?: A | null
+  b?: B | null
   returnVal: undefined
 }

maxDate

 maxDate<A extends DateValue, B extends DateValue> {
-  a: A
-  b: B
+  a?: A | null
+  b?: B | null
   returnVal: undefined
 }

@react-aria/calendar

AriaCalendarProps

 AriaCalendarProps<T extends DateValue> {
   autoFocus?: boolean = false
   defaultFocusedValue?: DateValue
   errorMessage?: ReactNode
   focusedValue?: DateValue
   isDateUnavailable?: (DateValue) => boolean
   isDisabled?: boolean = false
   isInvalid?: boolean
   isReadOnly?: boolean = false
-  maxValue?: DateValue
-  minValue?: DateValue
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
   onFocusChange?: (CalendarDate) => void
   pageBehavior?: PageBehavior = visible
 }

AriaRangeCalendarProps

 AriaRangeCalendarProps<T extends DateValue> {
   allowsNonContiguousRanges?: boolean
   autoFocus?: boolean = false
   defaultFocusedValue?: DateValue
   errorMessage?: ReactNode
   focusedValue?: DateValue
   isDateUnavailable?: (DateValue) => boolean
   isDisabled?: boolean = false
   isInvalid?: boolean
   isReadOnly?: boolean = false
-  maxValue?: DateValue
-  minValue?: DateValue
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
   onFocusChange?: (CalendarDate) => void
   pageBehavior?: PageBehavior = visible
 }

CalendarProps

-CalendarProps<T extends DateValue> {
-  autoFocus?: boolean = false
-  defaultFocusedValue?: DateValue
-  errorMessage?: ReactNode
-  focusedValue?: DateValue
-  isDateUnavailable?: (DateValue) => boolean
-  isDisabled?: boolean = false
-  isInvalid?: boolean
-  isReadOnly?: boolean = false
-  maxValue?: DateValue
-  minValue?: DateValue
-  onFocusChange?: (CalendarDate) => void
-  pageBehavior?: PageBehavior = visible
-}
+

RangeCalendarProps

-RangeCalendarProps<T extends DateValue> {
-  allowsNonContiguousRanges?: boolean
-  autoFocus?: boolean = false
-  defaultFocusedValue?: DateValue
-  errorMessage?: ReactNode
-  focusedValue?: DateValue
-  isDateUnavailable?: (DateValue) => boolean
-  isDisabled?: boolean = false
-  isInvalid?: boolean
-  isReadOnly?: boolean = false
-  maxValue?: DateValue
-  minValue?: DateValue
-  onFocusChange?: (CalendarDate) => void
-  pageBehavior?: PageBehavior = visible
-}
+

undefined

-
+CalendarProps<T extends DateValue | null> {
+  autoFocus?: boolean = false
+  defaultFocusedValue?: DateValue
+  errorMessage?: ReactNode
+  focusedValue?: DateValue
+  isDateUnavailable?: (DateValue) => boolean
+  isDisabled?: boolean = false
+  isInvalid?: boolean
+  isReadOnly?: boolean = false
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
+  onFocusChange?: (CalendarDate) => void
+  pageBehavior?: PageBehavior = visible
+}

undefined

-
+RangeCalendarProps<T extends DateValue | null> {
+  allowsNonContiguousRanges?: boolean
+  autoFocus?: boolean = false
+  defaultFocusedValue?: DateValue
+  errorMessage?: ReactNode
+  focusedValue?: DateValue
+  isDateUnavailable?: (DateValue) => boolean
+  isDisabled?: boolean = false
+  isInvalid?: boolean
+  isReadOnly?: boolean = false
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
+  onFocusChange?: (CalendarDate) => void
+  pageBehavior?: PageBehavior = visible
+}

@react-spectrum/calendar

Calendar

changed by:

  • Calendar
 SpectrumCalendarProps<T extends DateValue> {
   autoFocus?: boolean = false
   defaultFocusedValue?: DateValue
   errorMessage?: ReactNode
   focusedValue?: DateValue
   isDateUnavailable?: (DateValue) => boolean
   isDisabled?: boolean = false
   isInvalid?: boolean
   isReadOnly?: boolean = false
-  maxValue?: DateValue
-  minValue?: DateValue
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
   onFocusChange?: (CalendarDate) => void
   pageBehavior?: PageBehavior = visible
   visibleMonths?: number = 1
 }

it changed:

  • AnyCalendarDate
  • Calendar
  • AnyDateTime

RangeCalendar

 SpectrumRangeCalendarProps<T extends DateValue> {
   allowsNonContiguousRanges?: boolean
   autoFocus?: boolean = false
   defaultFocusedValue?: DateValue
   errorMessage?: ReactNode
   focusedValue?: DateValue
   isDateUnavailable?: (DateValue) => boolean
   isDisabled?: boolean = false
   isInvalid?: boolean
   isReadOnly?: boolean = false
-  maxValue?: DateValue
-  minValue?: DateValue
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
   onFocusChange?: (CalendarDate) => void
   pageBehavior?: PageBehavior = visible
   visibleMonths?: number = 1
 }

@react-stately/calendar

CalendarState

 CalendarState {
   focusNextDay: () => void
   focusNextPage: () => void
   focusNextRow: () => void
   focusNextSection: (boolean) => void
   focusPreviousDay: () => void
   focusPreviousPage: () => void
   focusPreviousRow: () => void
   focusPreviousSection: (boolean) => void
   focusSectionEnd: () => void
   focusSectionStart: () => void
   focusedDate: CalendarDate
   getDatesInWeek: (number, CalendarDate) => Array<CalendarDate | null>
   isCellDisabled: (CalendarDate) => boolean
   isCellFocused: (CalendarDate) => boolean
   isCellUnavailable: (CalendarDate) => boolean
   isDisabled: boolean
   isFocused: boolean
   isInvalid: (CalendarDate) => boolean
   isNextVisibleRangeInvalid: () => boolean
   isPreviousVisibleRangeInvalid: () => boolean
   isReadOnly: boolean
   isSelected: (CalendarDate) => boolean
   isValueInvalid: boolean
-  maxValue?: DateValue
-  minValue?: DateValue
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
   selectDate: (CalendarDate) => void
   selectFocusedDate: () => void
   setFocused: (boolean) => void
   setFocusedDate: (CalendarDate) => void
   timeZone: string
   value: CalendarDate | null
   visibleRange: RangeValue<CalendarDate>
 }
 

RangeCalendarState

 RangeCalendarState {
   anchorDate: CalendarDate | null
   focusNextDay: () => void
   focusNextPage: () => void
   focusNextRow: () => void
   focusNextSection: (boolean) => void
   focusPreviousDay: () => void
   focusPreviousPage: () => void
   focusPreviousRow: () => void
   focusPreviousSection: (boolean) => void
   focusSectionEnd: () => void
   focusSectionStart: () => void
   focusedDate: CalendarDate
   getDatesInWeek: (number, CalendarDate) => Array<CalendarDate | null>
   highlightDate: (CalendarDate) => void
-  highlightedRange: RangeValue<CalendarDate>
+  highlightedRange: RangeValue<CalendarDate> | null
   isCellDisabled: (CalendarDate) => boolean
   isCellFocused: (CalendarDate) => boolean
   isCellUnavailable: (CalendarDate) => boolean
   isDisabled: boolean
   isDragging: boolean
   isFocused: boolean
   isInvalid: (CalendarDate) => boolean
   isNextVisibleRangeInvalid: () => boolean
   isPreviousVisibleRangeInvalid: () => boolean
   isReadOnly: boolean
   isSelected: (CalendarDate) => boolean
   isValueInvalid: boolean
-  maxValue?: DateValue
-  minValue?: DateValue
+  maxValue?: DateValue | null
+  minValue?: DateValue | null
   selectDate: (CalendarDate) => void
   selectFocusedDate: () => void
   setAnchorDate: (CalendarDate | null) => void
   setDragging: (boolean) => void
   setFocused: (boolean) => void
   setFocusedDate: (CalendarDate) => void
-  setValue: (RangeValue<DateValue>) => void
+  setValue: (RangeValue<DateValue> | null) => void
   timeZone: string
-  value: RangeValue<DateValue>
+  value: RangeValue<DateValue> | null
   visibleRange: RangeValue<CalendarDate>
 }

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

5 participants