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.

Sign up for GitHub

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

Jump to bottom

[pickers] Clean component interfaces and remove non-implemented props #4758

Merged
merged 5 commits into from
May 5, 2022
Merged

[pickers] Clean component interfaces and remove non-implemented props #4758

merged 5 commits into from
May 5, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented May 4, 2022 •
edited
Loading

Closes #4557

In this PR

  • Remove onClose / onOpen and close props from the static pickers
  • Use the interfaces from usePickerState whenever possible rather than redefining its props
  • Remove ToolbarComponent prop from BaseDateRangePickerProps (not implemented, see [DateRangePicker] ToolbarComponent prop documented but not implemented #4557)
  • Remove toolbarPlaceholder prop from BaseDateRangePickerProps and BaseTimePickerProps (not implemented)
  • Remove toolbarFormat prop from BaseTimePickerProps (not implemented)

What's next ?

  • Move all the toolbar props in componentsProps.toolbar to avoid all this prop drilling
  • Try to simplify the component interface cascade (that one will be hard)

@flaviendelangle flaviendelangle added typescript component: pickers This is the name of the generic UI component, not the React module! labels May 4, 2022
@flaviendelangle flaviendelangle self-assigned this May 4, 2022
@mui-bot
Copy link

mui-bot commented May 4, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 267.1 542.1 379.3 374.54 96.089
Sort 100k rows ms 453.9 866.8 700 692.24 138.393
Select 100k rows ms 132 228.4 181.9 181.08 32.323
Deselect 100k rows ms 99.7 294 285 203.68 75.089

Generated by 🚫 dangerJS against 2960541

@flaviendelangle flaviendelangle changed the title [pickers] Clean interfaces [pickers] Clean component interfaces and remove non-implemented props May 4, 2022
flaviendelangle
flaviendelangle commented May 4, 2022
View reviewed changes
packages/x-date-pickers/src/internals/hooks/usePickerState.ts
@@ -96,13 +96,42 @@ interface DateStateAction<DraftValue> {
skipOnChangeCall?: boolean;
}

interface PickerStateProps<TInputValue, TValue> {
export interface PickerStateProps<TInputValue, TValue> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy paste of the descriptions in BasePickerProps

packages/x-date-pickers/src/internals/models/props/baseToolbarProps.ts
extends ExportedCalendarPickerProps<TDate>,
ExportedClockPickerProps<TDate> {
ExportedClockPickerProps<TDate>,
Omit<PickerStatePickerProps<TValue>, 'onDateChange'> {
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't pass onDateChange to the toolbar in CalendarOrClockPicker so I did not put it in the interface.
Instead the toolbar receives an onChange callback that is a small wrapper adding the variant arg.

I am in favor of removing this wrapper variant param in onDateChange, in which case we could just pass it to the toolbar and remove the onChange wrapper.

packages/x-date-pickers/src/internals/models/props/baseToolbarProps.ts

export interface BaseToolbarProps<TDate>
export interface BaseToolbarProps<TDate, TValue>
Copy link
Member Author

@flaviendelangle flaviendelangle May 4, 2022 •
edited by cherniavskii
Loading

Choose a reason for hiding this comment

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

I added a generic so that the date prop of BaseToolbarProps can be correctly typed with TValue (the prop will be renamed parsedValue in #4736)

flaviendelangle
flaviendelangle commented May 4, 2022
View reviewed changes
docs/pages/x/api/date-pickers/date-range-picker.json
@@ -2,6 +2,7 @@
"props": {
"onChange": { "type": { "name": "func" }, "required": true },
"renderInput": { "type": { "name": "func" }, "required": true },
"value": { "type": { "name": "arrayOf", "description": "Array&lt;any&gt;" }, "required": true },
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 don't understand why value was not in the proptypes

@flaviendelangle flaviendelangle marked this pull request as ready for review May 4, 2022 11:45
cherniavskii
cherniavskii reviewed May 4, 2022
View reviewed changes
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Great job reducing type confusions!

packages/x-date-pickers-pro/src/DateRangePicker/DateRangePicker.tsx Outdated Show resolved Hide resolved
@flaviendelangle flaviendelangle mentioned this pull request May 5, 2022
Closed
2 tasks
@flaviendelangle flaviendelangle merged commit 936655f into mui:master May 5, 2022
@flaviendelangle flaviendelangle deleted the ts-pickers branch May 5, 2022 09:42
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
@flaviendelangle @alexfauquette
[pickers] Clean component interfaces and remove non-implemented props (…
1216660 
…mui#4758)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cherniavskii cherniavskii cherniavskii approved these changes

Assignees

@flaviendelangle flaviendelangle

Labels
component: pickers This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DateRangePicker] ToolbarComponent prop documented but not implemented
3 participants
@flaviendelangle @mui-bot @cherniavskii