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] Limit the valid values of TDate #11791

Merged
merged 24 commits into from
Feb 5, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jan 23, 2024

Fixes #10540

image

The approach seems to work.
It will be breaking for people that don't import the adapter in the same typescript project.
For me it's an edge-case and they would just have to add the following import somewhere like they do for the theme augmentation:

import type {} from '@mui/x-date-pickers/AdapterDayjs';

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

mui-bot commented Jan 23, 2024

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running yarn l10n
  • Clean files with yarn prettier.

Deploy preview: https://deploy-preview-11791--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against d9c1ba1

@flaviendelangle flaviendelangle changed the title [pickers] POC: Limit the valid values of TDate [pickers] POC: Limit the valid values of TDate Jan 23, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2024
Copy link

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

@@ -278,7 +279,7 @@ DateTimePicker.propTypes = {
* The date used to generate the new value when both `value` and `defaultValue` are empty.
* @default The closest valid date-time using the validation props, except callbacks like `shouldDisable<...>`.
*/
referenceDate: PropTypes.any,
referenceDate: PropTypes.oneOfType([PropTypes.instanceOf(Date), PropTypes.object]),
Copy link
Member Author

Choose a reason for hiding this comment

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

OK this is the major drawback of the new approach..
The proptype generation script is smart with Date object does not just mark them as "object", so we end up with a proptype that is wrong for people not using date-fns...

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 could add a param in getPropTypesFromFile ignoreDateType or something similar, it's not great but I think it's worth it here

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this case would need some extra attention in order to avoid this particular change. 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 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 Jan 24, 2024
@@ -55,56 +55,56 @@ import {
import { PickersSectionListProps } from '../PickersSectionList';

export interface PickersComponentsPropsList {
MuiClock: ClockProps<unknown>;
MuiClock: ClockProps<PickerValidDate>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a big benefit
Now those theme entries are correctly typed 🤩

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

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

@flaviendelangle flaviendelangle changed the title [pickers] POC: Limit the valid values of TDate [pickers] Limit the valid values of TDate Jan 24, 2024
<MobileDateTimePicker
open
value={adapterToUse.date('2021-11-20T10:01:22')}
defaultValue={(params) => <TextField {...params} />}
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 link
Member

Choose a reason for hiding this comment

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

🤣

@@ -70,7 +70,7 @@ function GridEditDateCell({
field,
value,
colDef,
}: GridRenderEditCellParams<any, Date | string | null>) {
}: GridRenderEditCellParams<any, Date | null, string>) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it a fix of something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the date in the pickers can no longer be a string since v6 and the formatted value on the other hand is always a string

Copy link
Member

Choose a reason for hiding this comment

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

Letting @mui/xgrid know.
Maybe you need to add a mention of this fact somewhere as well as it could impact the users. 🤔
Clarifying that this is technically not BC, because it is already the case for v6. 🙈 😆

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this looks good to me 👍🏻

Copy link

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 Jan 25, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 26, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 26, 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 Jan 26, 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.

Really nice to see such an improvement regarding typing, great work! 🚀 👏
Hopefully it will avoid numerous cases of people being lost with types at first. 🙈

It will be breaking for people that don't import the adapter in the same typescript project.
For me it's an edge-case and they would just have to add the following import somewhere like they do for the theme augmentation:

Maybe we could add this mention in the migration guide? 🤔

@@ -70,7 +70,7 @@ function GridEditDateCell({
field,
value,
colDef,
}: GridRenderEditCellParams<any, Date | string | null>) {
}: GridRenderEditCellParams<any, Date | null, string>) {
Copy link
Member

Choose a reason for hiding this comment

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

Letting @mui/xgrid know.
Maybe you need to add a mention of this fact somewhere as well as it could impact the users. 🤔
Clarifying that this is technically not BC, because it is already the case for v6. 🙈 😆

docs/pages/x/api/date-pickers/date-range-picker-day.json Outdated Show resolved Hide resolved
<MobileDateTimePicker
open
value={adapterToUse.date('2021-11-20T10:01:22')}
defaultValue={(params) => <TextField {...params} />}
Copy link
Member

Choose a reason for hiding this comment

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

🤣

@flaviendelangle flaviendelangle force-pushed the poc-ts-tdate branch 2 times, most recently from 1cbc888 to ca1f66d Compare January 26, 2024 15:16
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 1, 2024
Copy link

github-actions bot commented Feb 1, 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 Feb 2, 2024
@flaviendelangle flaviendelangle merged commit 00b6b27 into mui:next Feb 5, 2024
15 checks passed
@flaviendelangle flaviendelangle deleted the poc-ts-tdate branch February 5, 2024 08:06
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Co-authored-by: alexandre <alex.fauquette@gmail.com>
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! typescript v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Generic type properties like minDate and maxDate have no associated interface when no value is bound
5 participants