[pickers] Use grid for modifying the layout #6900
Conversation
alexfauquette
added
component: pickers
These are the results for the performance tests:
|
|
|
||
| return ( | ||
| <LocalizationProvider dateAdapter={AdapterDayjs}> | ||
| <Stack sx={{ width: '100%' }} alignItems="center"> |
There was a problem hiding this comment.
| <Stack sx={{ width: '100%' }} alignItems="center"> | |
| <Stack sx={{ width: '100%' }} alignItems="center" spacing={2}> |
Maybe the solution would be to avoid having 2 ToggleButtonGroup one above the other and go for 1 ToggleButtonGroup + Switch.
There was a problem hiding this comment.
I took some inspiration from joy, but as you can see, I'm not a designer 🙈🥸
There was a problem hiding this comment.
Not a design either 😆 , and we have to keep the responsive aspect of the doc in mind
I prefer this orientation, but I would remove the shadow
There was a problem hiding this comment.
I mobile buttons move on top. I could reduce spacing for this case
|
@flaviendelangle Not already answered :) For this specific demo, the idea would be to replace the action bar by the shortcuts latter. But it brings a larger topic: element order For toolbar, views, and actions sounds obvious to have them in the given order. Potential solutionsSolution 1Add props to the Not a big fan because it might ends into an large list of props Solution 2Add prop Why not but it dos not cover the case of dev willing to add elements in the middle Solution 3Give more responsibility to const LayoutRoot = () =>
<div>
<Toolbar />
<ContentWrapper>
<Tabs />
{children}
</ ContentWrapper>
<ActionBar />
</ div>Such that when Overriding the Maybe the better solution but sounds too heavy for a dev who just want to move the shortcut from the left to the bottom. Or we should provide a really nice example reusing the |
packages/x-date-pickers/src/internals/components/PickersViewLayout/PickersViewLayout.tsx
Outdated
Show resolved
Hide resolved
Yes, I was naively propagating props to children which included |
packages/x-date-pickers/src/internals/components/PickersViewLayout/PickersViewLayout.tsx
Outdated
Show resolved
Hide resolved
| <PickersViewLayoutRoot className={clsx(className, classes.root)} ref={ref}> | ||
| <PickersViewLayoutContent className={classes.content}> | ||
| {shouldRenderToolbar && !!Toolbar && ( | ||
| const subComponents = { |
There was a problem hiding this comment.
Not sure it's a good way of doing
packages/x-date-pickers/src/PickersLayout/PickersLayout.types.ts
Outdated
Show resolved
Hide resolved
|
Maybe we should add |
|
@LukasTy yes, that's my bad |
|
Maybe we could override |
| // TODO: Remove this functions. It get introduced to mark `value` prop in PickersLayoutProps as not required. | ||
| // The true type should be | ||
| // - For pickers value: TDate | null | ||
| // - For rangepickers value: [TDate | null, TDate | null] | ||
| function toolbarHasValue<TValue, TView extends DateOrTimeView>( | ||
| toolbarProps: BaseToolbarProps<TValue | undefined, TView> | any, | ||
| ): toolbarProps is BaseToolbarProps<TValue, TView> { | ||
| return toolbarProps.value !== undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
Since the value: TValue of layout props is interpreted as Proptypes.any.require, it returns an error when we pass null, which is a valid value for pickers (not for range pickers)
Since I did not find any solution to this problem, I did the following trick: override it with
value?: TValue
To be compliant with props types expected by the toolbar, I added this toolbarHasValue to ensure value is not undefined and set the correct type
@LukasTy @flaviendelangle Do you agree with this nasty TS trick?
There was a problem hiding this comment.
I've got a question from different PoV: do we need value: TValue in toolbar instead of value?: TValue?
Pickers are fine working without any initial value defined. Do we need to force our selves so that internal components have to have this field defined marked as required if it causes us issues?
What do you think about the following diff?
diff --git a/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx b/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx
index e12140b14..5fe81985e 100644
--- a/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx
+++ b/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx
@@ -70,7 +70,7 @@ export const DateRangePickerToolbar = React.forwardRef(function DateRangePickerT
const props = useThemeProps({ props: inProps, name: 'MuiDateRangePickerToolbar' });
const {
- value: [start, end],
+ value,
isMobileKeyboardViewOpen,
toggleMobileKeyboardView,
rangePosition,
@@ -78,6 +78,7 @@ export const DateRangePickerToolbar = React.forwardRef(function DateRangePickerT
toolbarFormat,
className,
} = props;
+ const [start, end] = value ?? [null, null];
const localeText = useLocaleText<TDate>();
diff --git a/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx b/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
index bd0a75b6a..5008ddd57 100644
--- a/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
+++ b/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
@@ -13,16 +13,6 @@ function toolbarHasView<TValue, TView extends DateOrTimeView>(
return toolbarProps.view !== null;
}
-// TODO: Remove this functions. It get introduced to mark `value` prop in PickersLayoutProps as not required.
-// The true type should be
-// - For pickers value: TDate | null
-// - For rangepickers value: [TDate | null, TDate | null]
-function toolbarHasValue<TValue, TView extends DateOrTimeView>(
- toolbarProps: BaseToolbarProps<TValue | undefined, TView> | any,
-): toolbarProps is BaseToolbarProps<TValue, TView> {
- return toolbarProps.value !== undefined;
-}
-
const useUtilityClasses = (ownerState: PickersLayoutProps<any, any>) => {
const { classes, isLandscape } = ownerState;
const slots = {
@@ -104,10 +94,7 @@ const usePickerLayout = <TValue, TView extends DateOrTimeView>(
ownerState: { ...props, wrapperVariant },
});
const toolbar =
- toolbarHasView(toolbarProps) &&
- toolbarHasValue<TValue, TView>(toolbarProps) &&
- shouldRenderToolbar &&
- !!Toolbar ? (
+ toolbarHasView(toolbarProps) && shouldRenderToolbar && !!Toolbar ? (
<Toolbar {...toolbarProps} />
) : null;
diff --git a/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx b/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx
index 9cdda9ae5..b0f829dfb 100644
--- a/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx
+++ b/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx
@@ -125,7 +125,7 @@ export function TimePickerToolbar<TDate extends unknown>(inProps: TimePickerTool
const {
ampm,
ampmInClock,
- value,
+ value = null,
isLandscape,
isMobileKeyboardViewOpen,
onChange,
diff --git a/packages/x-date-pickers/src/internals/models/props/toolbar.ts b/packages/x-date-pickers/src/internals/models/props/toolbar.ts
index 5acd32d80..f721169ab 100644
--- a/packages/x-date-pickers/src/internals/models/props/toolbar.ts
+++ b/packages/x-date-pickers/src/internals/models/props/toolbar.ts
@@ -5,7 +5,7 @@ export interface BaseToolbarProps<TValue, TView extends DateOrTimeView>
extends ExportedBaseToolbarProps {
isLandscape: boolean;
onChange: (newValue: TValue) => void;
- value: TValue;
+ value?: TValue;
/**
* Currently visible picker view.
*/
Or maybe even making the value not required in the original type?
diff --git a/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx b/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx
index e12140b14..5fe81985e 100644
--- a/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx
+++ b/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerToolbar.tsx
@@ -70,7 +70,7 @@ export const DateRangePickerToolbar = React.forwardRef(function DateRangePickerT
const props = useThemeProps({ props: inProps, name: 'MuiDateRangePickerToolbar' });
const {
- value: [start, end],
+ value,
isMobileKeyboardViewOpen,
toggleMobileKeyboardView,
rangePosition,
@@ -78,6 +78,7 @@ export const DateRangePickerToolbar = React.forwardRef(function DateRangePickerT
toolbarFormat,
className,
} = props;
+ const [start, end] = value ?? [null, null];
const localeText = useLocaleText<TDate>();
diff --git a/packages/x-date-pickers/src/PickersLayout/PickersLayout.types.ts b/packages/x-date-pickers/src/PickersLayout/PickersLayout.types.ts
index 21dc3ad5a..67b2f7e5d 100644
--- a/packages/x-date-pickers/src/PickersLayout/PickersLayout.types.ts
+++ b/packages/x-date-pickers/src/PickersLayout/PickersLayout.types.ts
@@ -70,8 +70,7 @@ export interface PickersLayoutSlotsComponentsProps<TValue, TView extends DateOrT
}
export interface PickersLayoutProps<TValue, TView extends DateOrTimeView>
- extends Omit<UsePickerLayoutPropsResponseLayoutProps<TValue, TView>, 'value'> {
- value?: TValue;
+ extends UsePickerLayoutPropsResponseLayoutProps<TValue, TView> {
className?: string;
children?: React.ReactNode;
sx?: SxProps<Theme>;
diff --git a/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx b/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
index bd0a75b6a..5008ddd57 100644
--- a/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
+++ b/packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
@@ -13,16 +13,6 @@ function toolbarHasView<TValue, TView extends DateOrTimeView>(
return toolbarProps.view !== null;
}
-// TODO: Remove this functions. It get introduced to mark `value` prop in PickersLayoutProps as not required.
-// The true type should be
-// - For pickers value: TDate | null
-// - For rangepickers value: [TDate | null, TDate | null]
-function toolbarHasValue<TValue, TView extends DateOrTimeView>(
- toolbarProps: BaseToolbarProps<TValue | undefined, TView> | any,
-): toolbarProps is BaseToolbarProps<TValue, TView> {
- return toolbarProps.value !== undefined;
-}
-
const useUtilityClasses = (ownerState: PickersLayoutProps<any, any>) => {
const { classes, isLandscape } = ownerState;
const slots = {
@@ -104,10 +94,7 @@ const usePickerLayout = <TValue, TView extends DateOrTimeView>(
ownerState: { ...props, wrapperVariant },
});
const toolbar =
- toolbarHasView(toolbarProps) &&
- toolbarHasValue<TValue, TView>(toolbarProps) &&
- shouldRenderToolbar &&
- !!Toolbar ? (
+ toolbarHasView(toolbarProps) && shouldRenderToolbar && !!Toolbar ? (
<Toolbar {...toolbarProps} />
) : null;
diff --git a/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx b/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx
index 9cdda9ae5..b0f829dfb 100644
--- a/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx
+++ b/packages/x-date-pickers/src/TimePicker/TimePickerToolbar.tsx
@@ -125,7 +125,7 @@ export function TimePickerToolbar<TDate extends unknown>(inProps: TimePickerTool
const {
ampm,
ampmInClock,
- value,
+ value = null,
isLandscape,
isMobileKeyboardViewOpen,
onChange,
diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 077cabcea..564eb8496 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -203,7 +203,7 @@ export interface UsePickerValueViewsResponse<TValue> {
* Props passed to `usePickerLayoutProps`.
*/
export interface UsePickerValueLayoutResponse<TValue> extends UsePickerValueActions {
- value: TValue;
+ value?: TValue;
onChange: (newValue: TValue) => void;
}
diff --git a/packages/x-date-pickers/src/internals/models/props/toolbar.ts b/packages/x-date-pickers/src/internals/models/props/toolbar.ts
index 5acd32d80..f721169ab 100644
--- a/packages/x-date-pickers/src/internals/models/props/toolbar.ts
+++ b/packages/x-date-pickers/src/internals/models/props/toolbar.ts
@@ -5,7 +5,7 @@ export interface BaseToolbarProps<TValue, TView extends DateOrTimeView>
extends ExportedBaseToolbarProps {
isLandscape: boolean;
onChange: (newValue: TValue) => void;
- value: TValue;
+ value?: TValue;
/**
* Currently visible picker view.
*/
There was a problem hiding this comment.
Thanks for the different PoV, I may have spent too much time on Layout typing that I forget I could modify others 👍
Since toolbar value is not controlled, it could be undefined, and that's already a better solution than what I proposed
But the cleanest solution could be about the props type generator. I don't get why it set Required to a type any. If it is of type any, it can contain null or undefined and so it's not required
There was a problem hiding this comment.
But the cleanest solution could be about the props type generator. I don't get why it set
Requiredto a typeany. If it is of typeany, it can containnullorundefinedand so it's not required
You could look into this solution suggested by Olivier. 🤔
There was a problem hiding this comment.
Amazing work, @alexfauquette!
The new hook is elegant, and the abilities to customize the layout are simply great!
The exploration in collaboration with the team was also outstanding.
I left a few suggestions to trim the docs a bit and spotted an issue with the demos using the custom ActionList.
|
|
||
| ## Layout structure | ||
|
|
||
| The layout is structured as follows: |
There was a problem hiding this comment.
I think we can skip to the meat.
| The layout is structured as follows: |
|
|
||
| The layout is structured as follows: | ||
|
|
||
| A `<PickersLayoutRoot />` wraps all the sub-components to provide the structure. |
There was a problem hiding this comment.
| A `<PickersLayoutRoot />` wraps all the sub-components to provide the structure. | |
| A `<PickersLayoutRoot />` wraps all the subcomponents to provide the structure. |
There was a problem hiding this comment.
We are using sub-components in the side menu as well 🤔
There was a problem hiding this comment.
I saw both in use on the documentation page, so I did a quick research about it yesterday, and "subcomponent" (without hyphen) seems way more commonly used. But they are both correct.
In our docs, only MUI Base is consistently using it without, but it's also where Sam made his biggest contribution.
Generally speaking, I prefer the readability without the hyphen, but I'm fine with keeping the hyphen as well.
There was a problem hiding this comment.
Gotcha, I also don't have a strong preference, as long as we are more or less consistent. 👌
There was a problem hiding this comment.
Ok, then I will do the modification an enforce the consistency :)
|
|
||
| ## CSS customization | ||
|
|
||
| To move an element—the easiest way is to override it's wrapper position with [`gridColumn`](https://developer.mozilla.org/en-US/docs/Web/CSS/grid-column) and [`gridRow`](https://developer.mozilla.org/en-US/docs/Web/CSS/grid-row) properties. |
There was a problem hiding this comment.
| To move an element—the easiest way is to override it's wrapper position with [`gridColumn`](https://developer.mozilla.org/en-US/docs/Web/CSS/grid-column) and [`gridRow`](https://developer.mozilla.org/en-US/docs/Web/CSS/grid-row) properties. | |
| To move an element, you can override its position in the layout wrapper with [`gridColumn`](https://developer.mozilla.org/en-US/docs/Web/CSS/grid-column) and [`gridRow`](https://developer.mozilla.org/en-US/docs/Web/CSS/grid-row) properties. |
| In the previous demonstration, the tab order is broken, because the action bar appears before the calendar whereas in the DOM the action bar is still after the calendar. | ||
|
|
||
| To modify the DOM structure, you can create your own `Layout` component. | ||
| To simplify the job, use the `usePickerLayout` hook which takes Layout's props as an input and returns React nodes. |
There was a problem hiding this comment.
| To simplify the job, use the `usePickerLayout` hook which takes Layout's props as an input and returns React nodes. | |
| Use the `usePickerLayout` hook to get the subcomponents as React nodes. |
|
|
||
| This slot can also be used to add additional information in the layout. | ||
|
|
||
| Here is the previous demonstration with a fix for the tabulation order and logo added into the layout. |
There was a problem hiding this comment.
| Here is the previous demonstration with a fix for the tabulation order and logo added into the layout. | |
| Here is the complete example with a fix for the tabulation order and an external element added to the layout. |
| In the next example, the action bar is replaced by a list, and placed at the left of the component. | ||
| Placing at the right is achieved by applying `{ gridColumn: 1, gridRow: 2 }` style to it. | ||
|
|
||
| {{"demo": "MovingActions.js"}} |
There was a problem hiding this comment.
Changing the gridColumn and gridRow of the action bar on these demos has no effect.
I added the respective class to the custom ActionList to make the selector works.
<List className={pickersLayoutClasses.actionBar}>
There was a problem hiding this comment.
There's an open question left when using the pickersLayoutClasses.actionBar class to get the demo working. In this case, it had no negative effect, but it could potentially break something.
Maybe it's worth mentioning it in the docs (to call-out for attention), that when using custom components, you may need to define and use your own CSS class.
https://codesandbox.io/s/vibrant-platform-3c95zc?file=/demo.tsx
There was a problem hiding this comment.
It's a broader topic, about how to propagate correctly elements
I updated the demo as follow such that users can to customization directly in the docs, and added a warning such that they know about it
--- a/docs/data/date-pickers/custom-layout/MovingActions.tsx
+++ b/docs/data/date-pickers/custom-layout/MovingActions.tsx
@@ -10,7 +10,7 @@ import { Unstable_StaticNextDatePicker as StaticNextDatePicker } from '@mui/x-da
import { PickersActionBarProps } from '@mui/x-date-pickers/PickersActionBar';
function ActionList(props: PickersActionBarProps) {
- const { onAccept, onClear, onCancel, onSetToday } = props;
+ const { onAccept, onClear, onCancel, onSetToday, className } = props;
const actions = [
{ text: 'Accept', method: onAccept },
{ text: 'Clear', method: onClear },
@@ -18,7 +18,7 @@ function ActionList(props: PickersActionBarProps) {
{ text: 'Today', method: onSetToday },
];
return (
- <List>
+ <List className={className}>
{actions.map(({ text, method }) => (
<ListItem key={text} disablePadding>
|
I think that it's great to allow developers to assemble the different parts of the data grid as they see fit! The approach with |
|
The rational is that each of these component is a slot of it own |
|
This sounds fair. |
oliviertassinari
added
design
Documentation of the new layout is available at: https://deploy-preview-6900--material-ui-x.netlify.app/x/react-date-pickers/custom-layout/
Text is not polished but allows to have an overview
About adding elements in the layout, we could export sub-components, to allows a full DOM modification. But for most use-case, the original order make sense, and we add only add helping/description elements on top/bottom