-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[pickers] Rework view internals #7097
Conversation
These are the results for the performance tests:
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
"onMonthChange": { "type": { "name": "func" } }, | ||
"onViewChange": { "type": { "name": "func" } }, | ||
"onYearChange": { "type": { "name": "func" } }, | ||
"openTo": { | ||
"type": { | ||
"name": "enum", | ||
"description": "'day'<br>| 'month'<br>| 'year'" | ||
}, | ||
"default": "'day'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already lost this precision on the new pickers due to the shared interface.
It would be nice to be able to do something like:
/**
* @propDefaultValue view "day"
**/
interface MyComponentProps extends SomeShareInterface {}
Not sure about the exact synthax
This would allow to define doc default values without redefining the type itself.
If you know about any existing convention for this use case I'm very interested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific use case does not seem to be supported by jsdocs.
You have the list of "supported tags" here.
We could add a plugin to support an additional tag @overrideDefault
. But it sounds to be an interesting project, but will require some time to navigate int jsdocs documentation
@@ -55,7 +55,6 @@ | |||
"onError": { "type": { "name": "func" } }, | |||
"onMonthChange": { "type": { "name": "func" } }, | |||
"onOpen": { "type": { "name": "func" } }, | |||
"onViewChange": { "type": { "name": "func" } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop is never used (and removed on the new pickers)
I can add it back to the typing manually but I don't think it's worth it
Same on other legacy range pickers
@@ -1,5 +1,6 @@ | |||
{ | |||
"props": { | |||
"autoFocus": { "type": { "name": "bool" } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DateCalendar
already had this prop
No idea why the doc gen was not getting it
Same for DateRangeCalendar
and all the pickers that use them
@@ -14,19 +15,25 @@ | |||
"disablePast": { "type": { "name": "bool" } }, | |||
"displayWeekNumber": { "type": { "name": "bool" } }, | |||
"fixedWeekNumber": { "type": { "name": "number" }, "default": "undefined" }, | |||
"focusedView": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DateCalendar
already had this prop
No idea why the doc gen was not getting it
Same for onFocusedViewChange
@@ -81,6 +82,12 @@ | |||
} | |||
}, | |||
"value": { "type": { "name": "any" } }, | |||
"view": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how I missed it, but all the new pickers were missing the view
prop
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@@ -496,7 +496,7 @@ const DateRangeCalendar = React.forwardRef(function DateRangeCalendar<TDate>( | |||
{calendars === 1 ? ( | |||
<PickersCalendarHeader | |||
views={['day']} | |||
openView={'day'} | |||
view={'day'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PickersCalendarHeader
is exposed via the theme
But the typing only contains classes
so I would not count this one as a breaking change
@@ -44,45 +45,12 @@ export interface DateRangeCalendarSlotsComponentsProps<TDate> | |||
>; | |||
} | |||
|
|||
export interface DateRangeCalendarProps<TDate> | |||
export interface ExportedDateRangeCalendarProps<TDate> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic before:
interface DateRangeCalendarProps = { ... }
interface ExportedDateRangeCalendarProps extends Omit<DateRangeCalendarProps, ...> {}
The logic after:
interface ExportedDateRangeCalendarProps { ... }
interface DateRangeCalendarProps extends ExportedDateRangeCalendarProps { ... }
@@ -36,7 +36,7 @@ export interface BaseDatePickerSlotsComponentsProps<TDate> | |||
} | |||
|
|||
export interface BaseDatePickerProps<TDate> | |||
extends ExportedDateCalendarProps<TDate>, | |||
extends Omit<ExportedDateCalendarProps<TDate>, 'value' | 'onChange' | 'defaultValue'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calendars have the same value
/ onChange
/ defaultValue
as the new pickers
But for the legacy pickers, I have to omit here and re-define to avoid type conflicts
@@ -774,14 +774,6 @@ describe('<DesktopDatePicker />', () => { | |||
}); | |||
}); | |||
|
|||
it('should throw console warning when invalid `openTo` prop is provided', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one is interesting
The warnOnlyOnce
variables are not resetting between tests.
So the 2nd test failed.
I removed it since it's a legacy pickers.
But I don't know how we could cleanly test those warnings.
Maybe just export the variables from the internals
and reset them to false
after the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove the warnOnce
logic in test env
let warnedOnce = false;
if (!warnedOnce || process.env.NODE_ENV === 'test') {
console.warn(...)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid it would be logged several times and would make the test crash even with toWarnDev
(which would only catch the first one)
To be tested
@@ -28,26 +27,13 @@ export type PickerViewRendererLookup< | |||
/** | |||
* Props used to handle the views that are common to all pickers. | |||
*/ | |||
export interface UsePickerViewsBaseProps<TView extends DateOrTimeView> { | |||
export interface UsePickerViewsBaseProps<TView extends DateOrTimeView> | |||
extends Omit<UseViewsOptions<any, TView>, 'onChange' | 'onFocusedViewChange' | 'focusedView'> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not wanted to add controllable focused view on the picker on this PR.
But that would be very easy to do if we wanted to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that focus management get moved to the useView
@@ -61,7 +61,7 @@ export interface MonthCalendarProps<TDate> | |||
disableHighlightToday?: boolean; | |||
onMonthFocus?: (month: number) => void; | |||
hasFocus?: boolean; | |||
onFocusedViewChange?: (newHasFocus: boolean) => void; | |||
onFocusedViewChange?: (hasFocus: boolean) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the "new" it sounds more obvious that the function has to be call when the focus is modified. Not a strong opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main rational here was that on onChange
we are always calling it value
not newValue
in the interface
(example from the base
package)
onChange?: (event: React.SyntheticEvent, value: number | string) => void;
In this PR
setOpenView
=>setView
inuseViews
(and in any component passing this prop down we name itonViewChange
)openView
=>view
inuseViews
(and in any component passing this prop down)useFocusManagement
insideuseViews
and avoid havingsetFocusedView
return a function (we were not memoizing anything anyway)handleChangeAndOpenNext
=>setValueAndGoToNextView
inuseViews
(avoid mixingset
,handle
andon
for callbacks returned by a hook and make clear what we are changing and opening)openNext
=>goToNextView
(make clear that it is the view we are changing)useViews
to create the props of every component using it (was already the case for the new pickers, did it forDateCalendar
which causes some bug on the old picker because of type inconsistencies. For instance I had to redefine thesx
prop on non-static pickers. This change helps us migrate away forExportedDateCalendarProps = Omit<DateCalendarProps, ...>
which is a very error-prone pattern.Questions
openTo
=>defaultView
? On static pickers and calendars, this name make no sense, and naming isdefaultView
would make very clear that it behaves like avalue
/defaultValue
pattern.=> We are keeping
openTo
for nowopenTo
at the component level (useDatePickerDefaultizedProps
for instance) in case the current fallback is not in the views (for the date picker, it would be something likeviews.includes('day') ? 'day' : views[0]
. Because right now if we apply a custom set of views that do not containday
, we have to manually defineopenTo
, otherwise it breaks.=> Yes we should apply smart defaults