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] Always use the same timezone in the field, the view and the layout components #13481

Merged
merged 4 commits into from
Jun 14, 2024
Merged

[pickers] Always use the same timezone in the field, the view and the layout components #13481

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a12db4
[pickers] Always use the same timezone in the field, the view and the…
flaviendelangle Jun 14, 2024
006fe5e
Clean
flaviendelangle Jun 14, 2024
f188a4c
Add test
flaviendelangle Jun 14, 2024
dfb0623
Fix
flaviendelangle Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/x-date-pickers/src/PickersLayout/usePickerLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ const usePickerLayout = <
const classes = useUtilityClasses(props);

// Action bar

const ActionBar = slots?.actionBar ?? PickersActionBar;
const actionBarProps = useSlotProps({
elementType: ActionBar,
Expand All @@ -93,7 +92,6 @@ const usePickerLayout = <
const actionBar = <ActionBar {...actionBarProps} />;

// Toolbar

const Toolbar = slots?.toolbar;
const toolbarProps = useSlotProps({
elementType: Toolbar!,
Expand All @@ -114,19 +112,16 @@ const usePickerLayout = <
const toolbar = toolbarHasView(toolbarProps) && !!Toolbar ? <Toolbar {...toolbarProps} /> : null;

// Content

const content = children;

// Tabs

const Tabs = slots?.tabs;
const tabs =
view && Tabs ? (
<Tabs view={view} onViewChange={onViewChange} className={classes.tabs} {...slotProps?.tabs} />
) : null;

// Shortcuts

const Shortcuts = slots?.shortcuts ?? PickersShortcuts;
const shortcutsProps = useSlotProps({
elementType: Shortcuts!,
Expand All @@ -144,7 +139,6 @@ const usePickerLayout = <
wrapperVariant,
},
});

const shortcuts = view && !!Shortcuts ? <Shortcuts {...shortcutsProps} /> : null;

return {
Expand Down
56 changes: 34 additions & 22 deletions packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,19 +165,31 @@ export const usePickerValue = <
const {
onAccept,
onChange,
value: inValue,
value: inValueWithoutRenderTimezone,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using a super explicit variable name here to lower the risk of re-using the variable below instead of the one with the right timezone.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think that having inValue and inValueWithTimezone would not be enough? 🤔
Or even as we have everywhere else: inValue and value for the value returned from useValueWithTimezone..? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue with naming the value returned by useValueWithTimezone is that we have the values that are stored in the state (the other usages of useValueWithTimezone don't have this issue), so the value returned by useValueWithTimezone is still not the value the we should use for most of the usages.

So I'd prefer to keep some information on the variable returned by useValueWithTimezone saying that it's still an "input" value.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have so many values in this hook 😆

  • props.value
  • props.defaultValue
  • the value returned by useValueWithTimezone
  • dateState.draft
  • dateState.lastPublishedValue
  • dateState.lastComittedValue
  • dateState.lastControlledValue

Copy link
Member

Choose a reason for hiding this comment

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

Ok, if you feel that it improves the clarity, let's go with that approach. 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively I can rename inValueWithRenderTimezone into inValue since it's the one we use as "props.value" in 90% of the file

defaultValue: inDefaultValue,
closeOnSelect = wrapperVariant === 'desktop',
timezone: timezoneProp,
} = props;

const { current: defaultValue } = React.useRef(inDefaultValue);
const { current: isControlled } = React.useRef(inValue !== undefined);
const { current: isControlled } = React.useRef(inValueWithoutRenderTimezone !== undefined);

const {
timezone,
value: inValueWithTimezoneToRender,
handleValueChange,
} = useValueWithTimezone({
timezone: timezoneProp,
value: inValueWithoutRenderTimezone,
defaultValue,
onChange,
valueManager,
});

/* eslint-disable react-hooks/rules-of-hooks, react-hooks/exhaustive-deps */
if (process.env.NODE_ENV !== 'production') {
React.useEffect(() => {
if (isControlled !== (inValue !== undefined)) {
if (isControlled !== (inValueWithTimezoneToRender !== undefined)) {
console.error(
[
`MUI X: A component is changing the ${
Expand All @@ -191,7 +203,7 @@ export const usePickerValue = <
].join('\n'),
);
}
}, [inValue]);
}, [inValueWithTimezoneToRender]);

React.useEffect(() => {
if (!isControlled && defaultValue !== inDefaultValue) {
Expand All @@ -213,8 +225,8 @@ export const usePickerValue = <

const [dateState, setDateState] = React.useState<UsePickerValueState<TValue>>(() => {
let initialValue: TValue;
if (inValue !== undefined) {
initialValue = inValue;
if (inValueWithTimezoneToRender !== undefined) {
initialValue = inValueWithTimezoneToRender;
} else if (defaultValue !== undefined) {
initialValue = defaultValue;
} else {
Expand All @@ -225,19 +237,11 @@ export const usePickerValue = <
draft: initialValue,
lastPublishedValue: initialValue,
lastCommittedValue: initialValue,
lastControlledValue: inValue,
lastControlledValue: inValueWithTimezoneToRender,
hasBeenModifiedSinceMount: false,
};
});

const { timezone, handleValueChange } = useValueWithTimezone({
timezone: timezoneProp,
value: inValue,
defaultValue,
onChange,
valueManager,
});

useValidation(
{ ...props, value: dateState.draft, timezone },
validator,
Expand Down Expand Up @@ -297,21 +301,29 @@ export const usePickerValue = <
});

if (
inValue !== undefined &&
inValueWithTimezoneToRender !== undefined &&
(dateState.lastControlledValue === undefined ||
!valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
!valueManager.areValuesEqual(
utils,
dateState.lastControlledValue,
inValueWithTimezoneToRender,
))
) {
const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);
const isUpdateComingFromPicker = valueManager.areValuesEqual(
utils,
dateState.draft,
inValueWithTimezoneToRender,
);

setDateState((prev) => ({
...prev,
lastControlledValue: inValue,
lastControlledValue: inValueWithTimezoneToRender,
...(isUpdateComingFromPicker
? {}
: {
lastCommittedValue: inValue,
lastPublishedValue: inValue,
draft: inValue,
lastCommittedValue: inValueWithTimezoneToRender,
lastPublishedValue: inValueWithTimezoneToRender,
draft: inValueWithTimezoneToRender,
hasBeenModifiedSinceMount: true,
}),
}));
Expand Down