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.

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

[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

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jun 14, 2024

Fixes #13319

Looks like the fix is easier than expected

All the tests are passing, for me this is fixing the root issue since usePickerValue returns the value that will be used in all the pieces of the picker.
I first tried to pass the timezone to the Toolbar, but it's quite fragile since we could have the exact same problem in other parts of the UI (the shortcuts for instance).

@flaviendelangle flaviendelangle self-assigned this Jun 14, 2024
@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! labels Jun 14, 2024
@@ -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

@mui-bot
Copy link

mui-bot commented Jun 14, 2024

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

Generated by 🚫 dangerJS against dfb0623

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.

Nice! 🎉
It's awesome to see that this was just a small oversight after all. 🙌

WDYT, would still adding at least a test or two make sense? 🤔

@@ -165,19 +165,31 @@ export const usePickerValue = <
const {
onAccept,
onChange,
value: inValue,
value: inValueWithoutRenderTimezone,
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..? 🤔

@flaviendelangle flaviendelangle marked this pull request as ready for review June 14, 2024 12:29
@flaviendelangle
Copy link
Member Author

WDYT, would still adding at least a test or two make sense? 🤔

I added one very basic test that reproduces the issue 👍

@LukasTy LukasTy added the time-zone Issues about time zone management label Jun 14, 2024
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.

Great work, thank you! 🙏 🎉

@flaviendelangle flaviendelangle merged commit 959c9b4 into mui:master Jun 14, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the toolbar-timezone branch June 14, 2024 13:04
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! time-zone Issues about time zone management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Toolbar components are not using the set timezone
3 participants