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

[pickers] Strictly type the props a picker passes to its field, and migrate all the custom field demos accordingly #15197

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 30, 2024

Extracted from #15194
Part of #14496

I wasn't able to split this one in smaller pieces because updating the demos without updating the package results in TS issues and vice-versa 😢 .
I tried to be as detailed as possible in my comments / description to help the review.

In this PR

  • Remove the doc section describing how to type a custom field (the new doc will come in a follow up)
  • Stop using BaseSingleInputFieldProps and BaseMultiInputFieldProps to type slotProps.field (the JSDoc said to not use it for this purpose even before this PR 😬 ) and stop exporting them (they are now only used to create interfaces like DatePickerFieldProps).
  • Improve BaseSingleInputFieldProps and BaseMultiInputFieldProps to accurately match what the picker are passing, which one are nullable and which one have a default value applied.
  • Create PickersFieldSlotProps and PickersRangeFieldSlotProps interfaces to describe what slotProps.field can receive. Those props are very limited for now to only contain stuff people should actually pass (I did not add any props that are forwarded by the picker, like format to avoid having several way to achieve the same outcome. If you have some props that you think are missing please let me know).
  • Use the ValidateXXXProps interfaces in the interfaces like DatePickerFieldProps instead of UseDateFieldProps (this makes DatePickerFieldProps a lot more accurate, without props like defaultValue that are never defined).
  • Clean the props passed by the picker to the field (in the useSlotProps of the useXXXPicker hooks) to accurately match what the new typing expects.
  • Update all the demos in the custom-field page to match the new typing (I did a comment per demo to describe the changes, for the Joy UI I didn't try to migrate more than what was necessary since they are going away in the future).

Follow up

  • Write the doc for the new custom field DX

@flaviendelangle flaviendelangle added docs Improvements or additions to the documentation component: pickers This is the name of the generic UI component, not the React module! labels Oct 30, 2024
@flaviendelangle flaviendelangle self-assigned this Oct 30, 2024
@mui-bot
Copy link

mui-bot commented Oct 30, 2024

@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch 2 times, most recently from 19ee412 to 6410aaf Compare October 31, 2024 12:15
@@ -4,18 +4,14 @@ import useForkRef from '@mui/utils/useForkRef';
import { styled } from '@mui/material/styles';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DatePickerFieldProps

Copy link
Member

Choose a reason for hiding this comment

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

Looks immensely cleaner. 😱 💯

@@ -8,18 +8,15 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DateRangePickerFieldProps (with a small adaptation to work with multi input range field, I can create a DateRangePickerMultiInputFieldProps if you want that in the lib, my assumption that since we migrate away from multi input, it's not needed).
  • Replace BasePickersTextFieldProps with BaseMultiInputPickersTextFieldProps (BaseMultiInputPickersTextFieldProps and BaseSingleInputPickersTextFieldProps are now incompatible so BasePickersTextFieldProps can't exist, and since we are migrating away from multi input I think we don't have to support this abstraction)
  • Remove defaultValue and onError props which are never passed by the picker to the field so a field built to be used inside a picker doesn't have to support them

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the assumptions and the resulting compromises. 👍

@@ -10,22 +10,14 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DateRangePickerFieldProps
  • Replace BasePickersTextFieldProps with BaseSingleInputPickersTextFieldProps (BaseMultiInputPickersTextFieldProps and BaseSingleInputPickersTextFieldProps are now incompatible so BasePickersTextFieldProps can't exist, and since we are migrating away from multi input I think we don't have to support this abstraction)
  • Use usePickersContext to open the picker instead of passing a custom prop using slotProps

Copy link
Member

Choose a reason for hiding this comment

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

These changes look epic and way cleaner without the need to control nested slotProps. 🎉

@@ -16,17 +16,13 @@ import FormControl from '@mui/joy/FormControl';
import FormLabel from '@mui/joy/FormLabel';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DatePickerFieldProps

@@ -22,16 +22,13 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DateRangePickerFieldProps (with a small adaptation to work with multi input range field, I can create a DateRangePickerMultiInputFieldProps if you want that in the lib, my assumption that since we migrate away from multi input, it's not needed).
  • Remove defaultValue and onError props which are never passed by the picker to the field so a field built to be used inside a picker doesn't have to support them
  • Add support for unstableStartFieldRef / unstableEndFieldRef

@@ -21,20 +21,12 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DateRangePickerFieldProps
  • Use usePickersContext to open the picker instead of passing a custom prop using slotProps

@@ -36,7 +36,7 @@ function AutocompleteField(props: AutocompleteFieldProps) {
...other
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Add the validation error in the onChange context
  • Don't check if onChange exists since it always does when used inside a picker

@@ -2,7 +2,6 @@ import * as React from 'react';
import dayjs, { Dayjs } from 'dayjs';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Unify the wording by removing ValueStr everywhere in favor of InputValue
  • Don't check if onChange exists since it always does when used inside a picker
  • Remove defaultValue prop and assume value is always defined (which is the case when the field is used inside a picker)

@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch from 6410aaf to 59c8cd5 Compare October 31, 2024 12:27
@@ -3,67 +3,67 @@ import dayjs, { Dayjs } from 'dayjs';
import Button from '@mui/material/Button';
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Use DatePickerFieldProps
  • Use useParsedFormat to generate the label of the Button instead of setting the format in the label of the picker (the end result is the same)
  • Use usePickersContext to open the picker
  • Use useSplitFieldProps to be able to forward the props to the DOM
  • Use useValidation

@flaviendelangle flaviendelangle marked this pull request as ready for review October 31, 2024 12:40
@flaviendelangle flaviendelangle changed the title [docs] Migrate remaining demos to the new custom field DX [docs] Strictly type the props a picker passes to its field, and migrate all the custom field demos accordingly Oct 31, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2024
@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch from 7593495 to 2b77887 Compare October 31, 2024 15:02
@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch from 2b77887 to 33bdaff Compare October 31, 2024 15:18
Copy link

github-actions bot commented Nov 5, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 5, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 6, 2024
- If you are building a custom field for a Date Picker:

```diff
-import {
Copy link
Member Author

@flaviendelangle flaviendelangle Nov 6, 2024

Choose a reason for hiding this comment

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

The old code is always with the old generic (TDate, TValue and TSection), not the new version of BaseSingleInputFieldProps that people can't use anymore anyway).

Copy link

github-actions bot commented Nov 6, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 6, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 6, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 8, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 12, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 12, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Epic work and result of type simplification. 👏

Leaving some questions and nitpicks. 😉 🙈

P.S. WDYT about changing the PR title prefix to [pickers]?
It's slightly change to deliver a BC within a [docs] PR. 🙈 😆

@@ -4,18 +4,14 @@ import useForkRef from '@mui/utils/useForkRef';
import { styled } from '@mui/material/styles';
Copy link
Member

Choose a reason for hiding this comment

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

Looks immensely cleaner. 😱 💯

@@ -8,18 +8,15 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
Copy link
Member

Choose a reason for hiding this comment

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

I agree with the assumptions and the resulting compromises. 👍

@@ -10,22 +10,14 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
Copy link
Member

Choose a reason for hiding this comment

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

These changes look epic and way cleaner without the need to control nested slotProps. 🎉

packages/x-date-pickers/src/DatePicker/DatePicker.types.ts Outdated Show resolved Hide resolved
packages/x-date-pickers/src/internals/models/fields.ts Outdated Show resolved Hide resolved
packages/x-date-pickers/src/models/fields.ts Outdated Show resolved Hide resolved
@@ -4,10 +4,7 @@
{ "name": "ArrowDropDownIcon", "kind": "Variable" },
{ "name": "ArrowLeftIcon", "kind": "Variable" },
{ "name": "ArrowRightIcon", "kind": "Variable" },
{ "name": "BaseMultiInputFieldProps", "kind": "Interface" },
Copy link
Member

Choose a reason for hiding this comment

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

Should we list these in the changelog of the PR as well? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I'm waiting for the alpha 0 to be released to have a clearer vision of how we want to phrase those changes but I'll add them in the PR description 👍

@flaviendelangle
Copy link
Member Author

P.S. WDYT about changing the PR title prefix to [pickers]?
It's slightly change to deliver a BC within a [docs] PR. 🙈 😆

You are entirely right 😬
The BC was not planned and I did not update the title since 😆

@flaviendelangle flaviendelangle changed the title [docs] Strictly type the props a picker passes to its field, and migrate all the custom field demos accordingly [pickers] Strictly type the props a picker passes to its field, and migrate all the custom field demos accordingly Nov 13, 2024
@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch 3 times, most recently from bd54062 to 06f7bbc Compare November 13, 2024 15:41
@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch from 9b5af2f to 454f8d5 Compare November 14, 2024 08:25
@flaviendelangle flaviendelangle force-pushed the custom-field-finish-migration branch from 454f8d5 to e040f2e Compare November 14, 2024 08:32
@flaviendelangle flaviendelangle merged commit 16c8962 into mui:master Nov 14, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the custom-field-finish-migration branch November 14, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: pickers This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants