[pickers] Move the adapters interfaces to our repository #8412
Conversation
flaviendelangle
added
the
component: pickers
flaviendelangle
force-pushed
the
adapter-typing
branch
from
9d209ea
to
efdac9e
Compare
Netlify deploy previewNetlify deploy preview: https://deploy-preview-8412--material-ui-x.netlify.app/ Updated pagesNo updates. These are the results for the performance tests:
|
flaviendelangle
force-pushed
the
adapter-typing
branch
from
efdac9e
to
a7931fb
Compare
| @@ -120,7 +120,6 @@ export type { | |||
| BaseNonStaticPickerProps, | |||
| } from './models/props/basePickerProps'; | |||
| export type { BaseToolbarProps, ExportedBaseToolbarProps } from './models/props/toolbar'; | |||
| export type { MuiPickersAdapter } from './models/muiPickersAdapter'; | |||
There was a problem hiding this comment.
Just an idea: yes, it is our internals and we do not endorse using them externally as well as do not guarantee its API stability, but still, this could impact users, who have built their custom adapters. 🤔
Maybe releasing it (or keeping PR open until we see the need for a minor) with at least a minor bump would make more sense? 🤔
There was a problem hiding this comment.
Which part of this PR is impacting people who build their own adapter or use our adapters externally ?
If it's just the import path of the model MuiPickersAdapter, then it's in internals so I don't think we should care 👼
There was a problem hiding this comment.
Well, yes, only the export location changes, it's internals, so, shouldn't be a big deal, but was just nitpicking that, as per semver suggestions, such change should be released at least as a minor. If not for the exclusion of internal export, then for the addition of new imports.
But in any case, it's just food for thought and just to consider, I'm not saying that we shouldn't or can't do whatever we see best. 👌
There was a problem hiding this comment.
We can merge this PR for v6.1, with the 1st real feature that the pickers or the grid has 👍
| * @param {TDate} value The given date. | ||
| * @returns {number} The date of the given date. | ||
| */ | ||
| getDate(value: TDate): number; |
There was a problem hiding this comment.
Nitpick: This method name always puzzled me... Why date, when it's working with day... 🤷 🙈
We can't change the method name now, but maybe we could at least provide a correct JSDoc? 🤔
P.S. Dayjs also has this confusing difference, but at least their doc specifies the correct output.
There was a problem hiding this comment.
date comes from new Date().getDate() (which is not a great naming imho).
We can rename to a more explicit naming in v7 (getMonthDay for example).
There was a problem hiding this comment.
datecomes fromnew Date().getDate()(which is not a great naming imho).
Well, it just proves that naming things is hard. 😆
IMHO, I'd be in favor of 🙈
getDay;getDayOfWeek,getDayOfTheWeek, orgetWeekDay;
This is the 1st part of migrating the adapters inside our repository.
IUtilsfrom@date-ioIUtilsonMuiPickersAdapterMuiPickersAdapterin@mui/x-date-pickers/models