[pickers] Use slots in the mobile and desktop wrappers instead of `XXXComponent` and `XXXProps` #6381

flaviendelangle · 2022-10-04T12:58:31Z

Part of #4466
Needed for #6070

Doc preview of the DatePicker slots

Desktop

Replace props.PaperProps by componentsProps.desktopPaper (add add components.DesktopPaper for consistency)
Replace props.PopperProps by componentsProps.popper (and add components.Popper for consistency)
Replace props.TransitionComponent by components.DesktopTransition, it is not used on mobile ! (and add a componentsProps.transition for consistency)
Replace props.TrapFocusProps by componentsProps.desktopTrapFocus, this prop was missing from the public component signature btw (and add a components.DesktopTrapFocus for consistency)

Mobile

Replace props.DialogProps by componentsProps.Dialog (and add components.dialog for consistency)
Add components.MobileTransition and componentsProps.mobileTransition (in the tests we were using props.DialogProps.TransitionProps which was not consistent with the desktop transition which was using props.TransitionProps).
Add components.MobilePaper and componentsProps.mobilePaper (replaces [pickers] Use PaperProps in mobile pickers #6223)

Changelog

Breaking changes

All the props used by the mobile and desktop wrappers to override components or components props has been replaced by component slots. You can find more information about this pattern in the MUI Base documentation.

Some of the names have also been prefixed by desktop when it was unclear that the behavior was only applied on the desktop version of the pickers (or the responsive version when used on a desktop).

The DialogProps prop has been replaced by a dialog component props slot on a responsive an mobile pickers:

// Same on MobileDatePicker, DateTimePicker, MobileDateTimePicker, 
// TimePicker, MobileTimePicker, DateRangePicker and MobileDateRangePicker.
<DatePicker
-  DialogProps={{ backgroundColor: 'red' }}
+  componentsProps={{ dialog: { backgroundColor: 'red }}}
/>

The PaperProps prop has been replaced by a desktopPaper component props slot on all responsive and desktop pickers:

// Same on DesktopDatePicker, DateTimePicker, DesktopDateTimePicker, 
// TimePicker, DesktopTimePicker, DateRangePicker and DesktopDateRangePicker.
<DatePicker
-  PaperProps={{ backgroundColor: 'red' }}
+  componentsProps={{ desktopPaper: { backgroundColor: 'red }}}
/>

The PopperProps prop has been replaced by a popper component props slot on all responsive and desktop pickers:

// Same on DesktopDatePicker, DateTimePicker, DesktopDateTimePicker, 
// TimePicker, DesktopTimePicker, DateRangePicker and DesktopDateRangePicker.
<DatePicker
-  PopperProps={{ onClick: handleClick }}
+  componentsProps={{ popper: { onClick: handleClick }}}
/>

The TransitionComponent prop has been replaced by a DesktopTransition component slot on all responsive and desktop pickers:

// Same on DesktopDatePicker, DateTimePicker, DesktopDateTimePicker, 
// TimePicker, DesktopTimePicker, DateRangePicker and DesktopDateRangePicker.
<DatePicker
-  TransitionComponent={Fade}
+  components={{ DesktopTransition: Fade }}
/>

The TrapFocusProps prop has been replaced by a desktopTrapFocus component props slot on all responsive and desktop pickers:

// Same on DesktopDatePicker, DateTimePicker, DesktopDateTimePicker, 
// TimePicker, DesktopTimePicker, DateRangePicker and DesktopDateRangePicker.
<DatePicker
-  TrapFocusProps={{ isEnabled: () => false }}
+  componentsProps={{ desktopTrapFocus: { isEnabled: () => false }}}
/>

mui-bot · 2022-10-04T13:04:06Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	682.9	1,194.6	687.5	906.36	196.362
Sort 100k rows	ms	786.5	1,370.1	1,241	1,110.62	201.34
Select 100k rows	ms	302.9	343.1	324.4	324.62	13.007
Deselect 100k rows	ms	172.7	406.6	217.3	253.46	82.922

Generated by 🚫 dangerJS against 3508f9c

github-actions · 2022-10-06T11:56:27Z

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

alexfauquette

Sounds good. I'm just wondering is PaperContent is still relevant or is it can be replaced by desktopPaper and mobilePaper

Not related to this PR (was there before) but I saw it when reading slot doc preview. The api page ends with

The component cannot hold a ref.

Even if DatePicker are wrapped into a React.forwardRef

alexfauquette · 2022-10-10T08:33:28Z

packages/x-date-pickers/src/internals/components/PickersPopper.tsx

-              elevation={8}
-              ref={handlePaperRef}
+          <Transition {...TransitionProps} {...componentsProps?.desktopTransition}>
+            <Paper
+              {...paperProps}
              onClick={(event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
                onPaperClick(event);
-                if (onPaperClickProp) {
-                  onPaperClickProp(event);
-                }
+                paperProps.onClick?.(event);
              }}
              onTouchStart={(event: React.TouchEvent<HTMLDivElement>) => {
                onPaperTouchStart(event);
-                if (onPaperTouchStartProp) {
-                  onPaperTouchStartProp(event);
-                }
+                paperProps.onTouchStart?.(event);
              }}
              ownerState={{ ...ownerState, placement }}
-              className={classes.paper}
-              {...otherPaperProps}
            >
              <PaperContent {...componentsProps?.paperContent}>


Why not merging Paper and PaperContent in a single element?

If I remember correctly, the PaperContent being a Fragment was just to avoid breaking changes. Now we are allowed to break it's the opportunity

@LukasTy Was there another reason?

After many discussions and experimentations—let's isolate this PR only for the refactor of props -> slots.
Merging Paper and PaperContent is not trivial as Static pickers do not have a wrapping Paper.
And we also have the issue of possibly having difficulties changing component layout once we flatten the structure of sub-components.
I think then will be the time to revisit this topic and look into a way of having a homogenous approach towards overriding/slots.

alexfauquette · 2022-10-10T08:43:43Z

packages/x-date-pickers/src/internals/components/PickersModalDialog.tsx

+      PaperComponent={components?.MobilePaper}
+      PaperProps={componentsProps?.mobilePaper}


If we keep the contentPaper should it also apply on mobile?

Suggested change

PaperComponent={components?.MobilePaper}

PaperProps={componentsProps?.mobilePaper}

PaperComponent={components?.MobilePaper ?? components?.ContentPaper}

PaperProps={componentsProps?.mobilePaper ?? componentsProps?.contentPaper}

@LukasTy was in favor not to because the layout wanted are often different
I will explain better than me

PaperContent could possibly be added to the mobile pickers as well.
It could be up to the user to decide when to display it or not using CSS if it is needed.

packages/x-date-pickers/src/internals/components/PickersPopper.tsx

flaviendelangle · 2022-10-10T09:32:12Z

Pour le rôle de PaperContent, je laisse @LukasTy juger, il connait mieux le sujet 👍

LukasTy · 2022-10-10T10:41:50Z

package.json

@@ -50,7 +50,7 @@
    "test:performance:server": "serve test/performance -p 5001",
    "test:argos": "node ./scripts/pushArgos.js",
    "typescript": "lerna run --no-bail --parallel typescript",
-    "typescript:ci": "lerna run --concurrency 7 --no-bail --no-sort typescript",
+    "typescript:ci": "lerna run --concurrency 6 --no-bail --no-sort typescript",


What is the reason behind this change? 🤔

I have a 137 TS error (out of memory)
And I can't find how to fix it without reducing the concurrency.
If you have a better solution 😬

packages/x-date-pickers/src/internals/components/wrappers/WrapperProps.ts

LukasTy · 2022-10-10T15:14:25Z

packages/x-date-pickers/src/internals/components/PickersPopper.tsx

-              elevation={8}
-              ref={handlePaperRef}
+          <Transition {...TransitionProps} {...componentsProps?.desktopTransition}>
+            <Paper
+              {...paperProps}
              onClick={(event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
                onPaperClick(event);
-                if (onPaperClickProp) {
-                  onPaperClickProp(event);
-                }
+                paperProps.onClick?.(event);
              }}
              onTouchStart={(event: React.TouchEvent<HTMLDivElement>) => {
                onPaperTouchStart(event);
-                if (onPaperTouchStartProp) {
-                  onPaperTouchStartProp(event);
-                }
+                paperProps.onTouchStart?.(event);
              }}
              ownerState={{ ...ownerState, placement }}
-              className={classes.paper}
-              {...otherPaperProps}
            >
              <PaperContent {...componentsProps?.paperContent}>


After many discussions and experimentations—let's isolate this PR only for the refactor of props -> slots.
Merging Paper and PaperContent is not trivial as Static pickers do not have a wrapping Paper.
And we also have the issue of possibly having difficulties changing component layout once we flatten the structure of sub-components.
I think then will be the time to revisit this topic and look into a way of having a homogenous approach towards overriding/slots.

LukasTy · 2022-10-10T15:16:42Z

packages/x-date-pickers/src/internals/components/PickersModalDialog.tsx

+      PaperComponent={components?.MobilePaper}
+      PaperProps={componentsProps?.mobilePaper}


PaperContent could possibly be added to the mobile pickers as well.
It could be up to the user to decide when to display it or not using CSS if it is needed.

[pickers] Use slots in PickersPopper and PickersModalDialog

25f4eb9

flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Oct 4, 2022

flaviendelangle marked this pull request as draft October 4, 2022 12:58

flaviendelangle self-assigned this Oct 4, 2022

Work

0dd8480

flaviendelangle changed the title ~~[pickers] Use slots in PickersPopper and PickersModalDialog [WIP]~~ [pickers] Use slots in PickersPopper and PickersModalDialog [WIP] Oct 5, 2022

flaviendelangle added 17 commits October 5, 2022 08:57

Work

120adec

Work

e720789

Fix

26f847a

WOrk

d1a1298

Work

ddabf2b

Work

b129dda

Fix

70f28a7

Test

bf15709

Test

44bd1d7

Test

577a711

Test

6c51f8f

Test

0471f2c

Test

b9d7ff4

Test

33b09ef

Test

b77a5ca

Test

a22f4fe

Fix

1394d9d

flaviendelangle changed the title ~~[pickers] Use slots in PickersPopper and PickersModalDialog [WIP]~~ [pickers] Use slots in the mobile and desktop wrappers instead of XXXComponent and XXXProps Oct 5, 2022

flaviendelangle mentioned this pull request Oct 5, 2022

[pickers] Use PaperProps in mobile pickers #6223

Closed

flaviendelangle marked this pull request as ready for review October 5, 2022 09:32

flaviendelangle requested review from alexfauquette and LukasTy October 5, 2022 09:32

flaviendelangle added the breaking change label Oct 5, 2022

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

Merge

9a0b150

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

flaviendelangle added 2 commits October 6, 2022 14:25

Work

ba31b18

Merge branch 'next' into use-slots-in-popper

9182c20

This was referenced Oct 10, 2022

[pickers] Replace toolbar props by a component slot #6445

Merged

[pickers] Use components slots on every component #4466

Closed

alexfauquette reviewed Oct 10, 2022

View reviewed changes

Code review: Alex

e41baf2

Merge branch 'next' into use-slots-in-popper

38546be

LukasTy approved these changes Oct 10, 2022

View reviewed changes

flaviendelangle added 2 commits October 11, 2022 07:49

Merge branch 'next' into use-slots-in-popper

ee0995a

Code review: Lukas

3508f9c

flaviendelangle merged commit e18970e into mui:next Oct 11, 2022

flaviendelangle deleted the use-slots-in-popper branch October 11, 2022 06:44

LukasTy mentioned this pull request Nov 1, 2022

[core] Fix typescript:ci failures #6705

Merged

LukasTy added the v6.x label Nov 25, 2022

LukasTy mentioned this pull request Nov 25, 2022

[pickers] Add codemods for breaking changes #6997

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Use slots in the mobile and desktop wrappers instead of `XXXComponent` and `XXXProps` #6381

[pickers] Use slots in the mobile and desktop wrappers instead of `XXXComponent` and `XXXProps` #6381

flaviendelangle commented Oct 4, 2022 •

edited

Loading

mui-bot commented Oct 4, 2022 •

edited

Loading

github-actions bot commented Oct 6, 2022

alexfauquette left a comment

alexfauquette Oct 10, 2022

LukasTy Oct 10, 2022

alexfauquette Oct 10, 2022

flaviendelangle Oct 10, 2022

LukasTy Oct 10, 2022

flaviendelangle commented Oct 10, 2022

LukasTy Oct 10, 2022

flaviendelangle Oct 11, 2022

LukasTy Oct 10, 2022

LukasTy Oct 10, 2022

		PaperComponent={components?.MobilePaper}
		PaperProps={componentsProps?.mobilePaper}

[pickers] Use slots in the mobile and desktop wrappers instead of XXXComponent and XXXProps #6381

[pickers] Use slots in the mobile and desktop wrappers instead of XXXComponent and XXXProps #6381

Conversation

flaviendelangle commented Oct 4, 2022 • edited Loading

Desktop

Mobile

Changelog

Breaking changes

mui-bot commented Oct 4, 2022 • edited Loading

github-actions bot commented Oct 6, 2022

alexfauquette 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

flaviendelangle commented Oct 10, 2022

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

[pickers] Use slots in the mobile and desktop wrappers instead of `XXXComponent` and `XXXProps` #6381

[pickers] Use slots in the mobile and desktop wrappers instead of `XXXComponent` and `XXXProps` #6381

flaviendelangle commented Oct 4, 2022 •

edited

Loading

mui-bot commented Oct 4, 2022 •

edited

Loading