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] Replace TValue and TSection generics with TIsRange #15290

Closed
wants to merge 17 commits into from
Closed

[pickers] Replace TValue and TSection generics with TIsRange #15290

wants to merge 17 commits into from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 5, 2024 •
edited
Loading

Replaced with #15434
Part of #14823 (step 2)

I will handle the migration guide in a follow up 👍

Example

 UseFieldInternalProps<
-  PickerRangeValue,
-  RangeFieldSection,
+  true,
   TEnableAccessibleFieldDOMStructure,
   TimeRangeValidationError
 >

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

mui-bot commented Nov 5, 2024 •
edited
Loading

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

Generated by 🚫 dangerJS against d1ca6a4

@flaviendelangle flaviendelangle marked this pull request as ready for review November 6, 2024 11:19
@github-actions GitHub Actions
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
flaviendelangle
flaviendelangle commented Nov 7, 2024
View reviewed changes
.../x-date-pickers-pro/src/MobileDateRangePicker/tests/describes.MobileDateRangePicker.test.tsx
@@ -45,7 +46,7 @@ describe('<MobileDateRangePicker /> - Describes', () => {
],
}));

describeValue(MobileDateRangePicker, () => ({
describeValue<true, 'picker'>(MobileDateRangePicker, () => ({
Copy link
Member Author

Choose a reason for hiding this comment

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

Before the value was any (since TValue doesn't have any extend attached to it).
But now that we have a good typing, it complains that value has the type PickerValue | PickerRangeValue which is not compatible with a lot of the things we do in setNewValue.
So I added manual generics to fix that.

@flaviendelangle flaviendelangle mentioned this pull request Nov 8, 2024
Open
2 tasks
@github-actions GitHub Actions
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 Nov 12, 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 12, 2024
LukasTy
LukasTy reviewed Nov 13, 2024
View reviewed changes
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.

I feel slightly unsure about these changes. 🤔
Passing only a boolean into a generic type feels slightly "over-optimized". 🙈

Sometimes a bit of explicitness is warranted.

Are you sure about this direction?

packages/x-date-pickers-pro/src/internals/hooks/useMultiInputFieldSelectedSections.ts Outdated Show resolved Hide resolved
packages/x-date-pickers/src/internals/models/value.ts
: PickerRangeValue
: PickerValue;

export type InferNonNullablePickerValue<TIsRange extends boolean> = TIsRange extends true
Copy link
Member

Choose a reason for hiding this comment

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

The fact that we need to put this logic and the RangePosition in the community package raises some "concern flags" in my head about the direction of the solution. 🙈

scripts/x-date-pickers.exports.json Outdated Show resolved Hide resolved
@flaviendelangle
Copy link
Member Author

I feel slightly unsure about these changes. 🤔

We can discuss it in the coming days.
I'll try to summarize why I think it improves the readability and the reliability of the codebase 👍

@github-actions GitHub Actions
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 Nov 14, 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 14, 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 15, 2024
@github-actions GitHub Actions
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 15, 2024
@flaviendelangle flaviendelangle mentioned this pull request Nov 15, 2024
Merged
@flaviendelangle flaviendelangle deleted the tisrange branch November 19, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasTy LukasTy LukasTy left review comments

@arthurbalduini arthurbalduini Awaiting requested review from arthurbalduini

Assignees

@flaviendelangle flaviendelangle

Labels
breaking change 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.
3 participants
@flaviendelangle @mui-bot @LukasTy