[pickers] Remove code duplication for the multi input range fields #15505

flaviendelangle · 2024-11-20T11:06:36Z

I'm doing this refacto now because it will make a lot easier to apply heavy change to those components and move the opening logic to be handled by the field instead of the picker.

Create a new public unstable hook useMultiInputRangeField that replaces useMultiInputDateRangeField, useMultiInputTimeRangeField and useMultiInputDateTimeRangeField (with improved DX)
Create a new internal function createMultiInputRangeField that create a multi input range field for Material
Use createMultiInputRangeField to create MultiInputDateRangeField, MultiInputTimeRangeField and MultiInputDateTimeRangeField
Clean the exported interfaces (no more MultiInputRangeFieldClasses exported, each component has it's interfaces such as MultiInputDateRangeFieldClasses, like any other regular component)

Extracted PRs

[docs] Fix inconsistent multi input range field separators #16043

github-actions · 2025-01-14T06:37:35Z

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

LukasTy

Superb improvement, especially codebase wise! 👍 💯
Leaving one problematic comment and some nitpicks. 😃

Clean the exported interfaces (no more MultiInputRangeFieldClasses exported, each component has it's interfaces such as MultiInputDateRangeFieldClasses, like any other regular component)
My main concern—do we need this change? What does it improve? 🤔

LukasTy · 2025-01-14T13:29:26Z

packages/x-date-pickers-pro/src/MultiInputDateRangeField/multiInputDateRangeFieldClasses.ts

+
+export const multiInputDateRangeFieldClasses: MultiInputRangeFieldClasses = generateUtilityClasses(
+  'MuiMultiInputDateRangeField',
+  ['root', 'separator'],


Have we considered making the textField as a slot as well? 🤔

I guess we did not because people could use the PickersTextField (or TextField from the core for the old DOM structure) classes directly.

If you think it's worth doing, then we can create an issue to do a follow up since it's not directly related to this PR.

It's debatable. 🤔
For consistent styling, yes, users can and should specify styles on the "root" component overrides (i.e. PickersTextField), but I can see use cases where someone would want to style specifically range Picker inputs differently. 🤷
But should we do it before there is demand for it? I'm not certain...
@noraleonte have you felt such a need when exploring styling customization for Pickers? 🤔

I don't have a very strong opinion on this 🤔 I guess a use case where overriding could be an app like Booking, Airbnb etc where you have forms in your app with inputs styled a certain way, but you want your app's main feature - the range picker - to have a different styling.
Targeting classes from the root works fine, since it's just one level of nesting. But having a specific slot might be a better DX generally 🤔

LukasTy · 2025-01-14T18:03:45Z

packages/x-date-pickers-pro/src/themeAugmentation/overrides.d.ts

+  MuiMultiInputDateRangeField: MultiInputDateRangeFieldClassKey;	
+  MuiMultiInputDateTimeRangeField: MultiInputDateTimeRangeFieldClassKey;	


Do you think it's worth having a separate type per field type, even though they are all identical and essentially just reexport exactly that in the original files. 🙈 🤷
I feel like it is a bit redundant... Also, close to no point in exporting them. 🤔

Suggested change

MuiMultiInputDateRangeField: MultiInputDateRangeFieldClassKey;

MuiMultiInputDateTimeRangeField: MultiInputDateTimeRangeFieldClassKey;

MuiMultiInputDateRangeField: MultiInputDateRangeFieldClassKey;

MuiMultiInputDateTimeRangeField: MultiInputDateTimeRangeFieldClassKey;

My goal here was to make those components behave like any other component in our packages.
Every component has named XXX has a XXXClassKey and XXXClasses exposed. And it has a MuiXXX entry in the theme.

And I don't think the reduced amount of exported variables is worth the mental overhead of having those components behaving differently.

WDYT?

Those are valid arguments. 👍
As for XXXClasses exports - I completely agree.
But for the XXXClassKey... I'm not sure if we should even be exporting them. 🙈
They seem mostly used/reserved for declaring it here, and we could do the following instead: 🤷

-MuiMultiInputDateRangeField: MultiInputDateRangeFieldClassKey; +MuiMultiInputDateRangeField: keyof MultiInputDateRangeFieldClasses;

AFAIK we are exporting the XXXClassKey purely because @mui/material does.
And unless I'm missing some rational, I agree with you that it's a loooooooooot of exports just to avoid a keyof for the few people that are using it 😆

If you're in favor of dropping all the XXXClassKey in our packages, we can double check with the core that we didn't miss a good reason not to.
And then plan it for v9.

...te-pickers-pro/src/internals/utils/createMultiInputRangeField/createMultiInputRangeField.tsx

packages/x-date-pickers-pro/src/hooks/useMultiInputRangeField/useMultiInputRangeField.ts

LukasTy · 2025-01-14T18:29:50Z

packages/x-date-pickers-pro/src/hooks/useMultiInputRangeField/useMultiInputRangeField.ts

+      value: valueProp === undefined ? undefined : valueProp[0],
+      defaultValue: defaultValue === undefined ? undefined : defaultValue[0],
+      onChange: handleStartDateChange,
+      autoFocus, // Do not add on end field.


This prop passing looks broken. 🙈
It end up on the Stack element:

The autoFocus was weirdly handled by the field, I moved it to the field internal props to mimic readOnly and disabled. It seems to work well, even on single input fields but I'll double check 👍

The autoFocus behavior seems somewhat broken on v7 as well. 🤔
But we are now breaking one behavior - autoFocus on a enableAccessibleFieldDOMStructure={false} SingleInput field. 🙈
This case works on v7.
As for other cases (DateRangePicker or MultiInputDateRangeField with enableAccessibleFieldDOMStructure={false} autoFocus) - they are also broken on v7, so it's not a blocker and can be fixed separately. 👌

Wow standalone field autoFocus is totally broken with the old DOM structure indeed...

But do you have a scenario where it works before this PR and does not work after?
For me standalone single input range field + autoFocus is broken both before and after.

But do you have a scenario where it works before this PR and does not work after?

Nevermind, I must have mixed something up... My previously mentioned case seems to work with both enableAccessibleFieldDOMStructure flag versions. 👌

docs/data/migration/migration-pickers-v7/migration-pickers-v7.md

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

…utRangeField/createMultiInputRangeField.tsx Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

…ge-picker' into use-managert-for-multi-input-range-picker

LukasTy

Looks great, apart from a small problem with the mentioned autoFocus behavior. 👍

packages/x-date-pickers/src/hooks/useSplitFieldProps.ts

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

LukasTy · 2025-01-15T12:13:27Z

packages/x-date-pickers-pro/src/hooks/useMultiInputRangeField/useMultiInputRangeField.ts

+      value: valueProp === undefined ? undefined : valueProp[0],
+      defaultValue: defaultValue === undefined ? undefined : defaultValue[0],
+      onChange: handleStartDateChange,
+      autoFocus, // Do not add on end field.


But do you have a scenario where it works before this PR and does not work after?

Nevermind, I must have mixed something up... My previously mentioned case seems to work with both enableAccessibleFieldDOMStructure flag versions. 👌

flaviendelangle added 30 commits November 5, 2024 19:07

[pickers] Replace TValue and TSection generics with TIsRange

5d145b6

Merge branch 'master' into tisrange

a8f316d

Fix CI

6ed4952

Merge

5904738

Merge branch 'master' into tisrange

5419886

Merge branch 'master' into tisrange

5c34425

[pickers] Introduce an improved version of the value manager

bbd4fe2

Work on value manager usage

7821632

Merge

3a73dd3

Merge branch 'tisrange' into v8-value-manager

dc89d3c

Work

eda4787

Fix

f7ee409

Merge branch 'master' into tisrange

b04e5bb

Work

cae8762

Work

f06c8ca

Merge branch 'tisrange' into v8-value-manager

48c9261

Fix'

20805bd

Work

f6b78d9

Fix

d47da98

Merge

7eebeb7

Merge branch 'tisrange' into v8-value-manager

3493484

Review: Lukas

3a52a82

Review: Lukas

3e18240

Remove as

8aae145

Merge branch 'tisrange' into v8-value-manager

49b7030

Move value manager to hookss

ae71f10

xImprove internals

4dbfd03

Merge branch 'master' into tisrange

6993047

Fix

9e7e9aa

Merge branch 'master' into tisrange

0d6562c

Fix

e3629be

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2025

flaviendelangle added 2 commits January 10, 2025 14:57

Merge branch 'master' into use-managert-for-multi-input-range-picker

b04de7b

Merge branch 'master' into use-managert-for-multi-input-range-picker

83af35e

flaviendelangle requested a review from LukasTy January 13, 2025 18:14

flaviendelangle marked this pull request as ready for review January 13, 2025 18:14

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 14, 2025

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 14, 2025

flaviendelangle force-pushed the use-managert-for-multi-input-range-picker branch from 64d46b7 to 86d236b Compare January 14, 2025 07:12

Merge

70fdc9b

flaviendelangle force-pushed the use-managert-for-multi-input-range-picker branch from 86d236b to 70fdc9b Compare January 14, 2025 07:14

flaviendelangle mentioned this pull request Jan 14, 2025

[pickers] Move the opening logic to the range fields #16175

Draft

LukasTy reviewed Jan 14, 2025

View reviewed changes

flaviendelangle and others added 8 commits January 15, 2025 10:24

Update docs/data/migration/migration-pickers-v7/migration-pickers-v7.md

0ae04a6

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

Update docs/data/migration/migration-pickers-v7/migration-pickers-v7.md

9d9a488

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

Update docs/data/migration/migration-pickers-v7/migration-pickers-v7.md

4c38c14

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

Update packages/x-date-pickers-pro/src/internals/utils/createMultiInp…

be00d3b

…utRangeField/createMultiInputRangeField.tsx Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

Merge branch 'master' into use-managert-for-multi-input-range-picker

29be344

Review: Lukas

d80c40d

Merge remote-tracking branch 'origin/use-managert-for-multi-input-ran…

a7654f2

…ge-picker' into use-managert-for-multi-input-range-picker

Move autoFocus to the field internal props

f860cc9

flaviendelangle force-pushed the use-managert-for-multi-input-range-picker branch from 79841ce to f860cc9 Compare January 15, 2025 09:40

LukasTy reviewed Jan 15, 2025

View reviewed changes

packages/x-date-pickers/src/hooks/useSplitFieldProps.ts Outdated Show resolved Hide resolved

LukasTy added the v8.x label Jan 15, 2025

Update packages/x-date-pickers/src/hooks/useSplitFieldProps.ts

bbac1c1

Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>

flaviendelangle requested a review from LukasTy January 15, 2025 11:47

LukasTy approved these changes Jan 15, 2025

View reviewed changes

Merge branch 'master' into use-managert-for-multi-input-range-picker

84ba6e9

flaviendelangle merged commit 90751bb into mui:master Jan 15, 2025
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Remove code duplication for the multi input range fields #15505

[pickers] Remove code duplication for the multi input range fields #15505

flaviendelangle commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Jan 14, 2025

LukasTy left a comment

LukasTy Jan 14, 2025

flaviendelangle Jan 15, 2025

LukasTy Jan 15, 2025

noraleonte Jan 15, 2025

LukasTy Jan 14, 2025

flaviendelangle Jan 15, 2025

LukasTy Jan 15, 2025

flaviendelangle Jan 15, 2025

LukasTy Jan 14, 2025

flaviendelangle Jan 15, 2025

LukasTy Jan 15, 2025

flaviendelangle Jan 15, 2025

LukasTy Jan 15, 2025

LukasTy left a comment

LukasTy Jan 15, 2025

		MuiMultiInputDateRangeField: MultiInputDateRangeFieldClassKey;
		MuiMultiInputDateTimeRangeField: MultiInputDateTimeRangeFieldClassKey;

[pickers] Remove code duplication for the multi input range fields #15505

[pickers] Remove code duplication for the multi input range fields #15505

Conversation

flaviendelangle commented Nov 20, 2024 • edited Loading

Extracted PRs

github-actions bot commented Jan 14, 2025

LukasTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaviendelangle commented Nov 20, 2024 •

edited

Loading