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

Date picker component #1499

Merged
merged 4 commits into from
Dec 30, 2022
Merged

Date picker component #1499

merged 4 commits into from
Dec 30, 2022

Conversation

bytasv
Copy link
Contributor

@bytasv bytasv commented Dec 29, 2022

CleanShot.2022-12-29.at.17.21.18.mp4

@bytasv bytasv requested a review from a team December 29, 2022 15:22
@bytasv bytasv self-assigned this Dec 29, 2022
@oliviertassinari oliviertassinari requested a deployment to date-picker-component - toolpad-db PR #1499 December 29, 2022 15:22 — with Render Abandoned
@oliviertassinari oliviertassinari temporarily deployed to date-picker-component - toolpad PR #1499 December 30, 2022 08:31 — with Render Destroyed
@bytasv bytasv merged commit 1c6877f into master Dec 30, 2022
@bytasv bytasv deleted the date-picker-component branch December 30, 2022 10:05
defaultValue: '',
defaultValueProp: 'defaultValue',
},
format: {
Copy link
Member

Choose a reason for hiding this comment

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

Currently, the Toolpad app developer decides for every user which format they have to input dates in. I wonder if it rather makes more sense that users input the date in the format based on their locale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder if it rather makes more sense that users input the date in the format based on their locale

I'm not really sure how that would work if say 2 users with different locales edited app, which one should be used? Or is there a possibility that DD-MM-YYYY on one machine might get converted to MM-DD-YYYY on the other? Also what if I don't want to use my locale because I'm building an app for users in a different locale?
IMO having an option to choose custom format should be among something that user is able to control, even retool has it:
CleanShot 2023-01-02 at 11 54 07@2x

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure how that would work if say 2 users with different locales edited app, which one should be used?

Each user in the locale of their browser. The value prop should be kept in an unambiguous format so that it doesn't matter to the runtime.

Also what if I don't want to use my locale because I'm building an app for users in a different locale?

Those end users will have their browsers set to their own locale, so if the date input just follows that, then it should be fine. What if I build applications for users in mixed locales? e.g. like in MUI: I, in Belgium should see "31/12/2022", and someone in US should see 12-31-2022, and the value prop should be 2022-12-31 for both cases

IMO having an option to choose custom format should be among something that user is able to control

Which user, the app editor? or the end user? in any case, we can always do both options, default to "use end user locale" and allow overriding with a custom format (and we can probably consolidate the separator and format props into 1 "format" prop).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value prop should be kept in an unambiguous format so that it doesn't matter to the runtime.

When I use datePicker.value in a text field somewhere do we display raw value or formatted with locale? If formatted - do we check all values if it matches date string or shall we also use some meta data from datepicker field to know that we are dealing with date value?

in any case, we can always do both options, default to "use end user locale" and allow overriding with a custom format

Do I understand correctly that you'd like to have format input field which is free form text field allowing to pass any format that is supported and if nothing is provided default to locale format?

Copy link
Member

Choose a reason for hiding this comment

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

When I use datePicker.value in a text field somewhere do we display raw value or formatted with locale?

I believe we should keep this value unformatted. i.e. a shorthand version of ISO8601 should be fine (2022-12-31). I think the most important is that this value is unambiguous, I should be able to do new Date(datePicker.value) and that should interpret the correct date everywhere. This is important because often these values will be sent back to the server, and will then lose all the context of the end user locale.
If the user wants to display such a value somewhere in the UI, for now they'll have to expose a format function on the global scope and use that to display the date. Fully aware this has more friction than it could have, but we can iterate on that in a separate effort.

Do I understand correctly that you'd like to have format input field which is free form text field allowing to pass any format that is supported and if nothing is provided default to locale format?

It's an option yes, my preference would be free form text, leave empty to use end user locale, default empty. But we can iterate on the idea.

value: {
typeDef: { type: 'string' },
onChangeProp: 'onChange',
onChangeHandler: (newValue: Dayjs, { format, separator }: Props) => {
Copy link
Member

Choose a reason for hiding this comment

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

For uniformity, I prefer to keep the convention of

value: T
onChange: (newValue: T) => void

I prefer to not ad hoc update the signature of onChangeHandler until we have more compelling use-cases to do so.

I think we should standardize on toISOString() for the format of value and newValue, so that the developer doesn't have to consider the formatting when handling this date.

for a benchmark, refer to retool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For uniformity, I prefer to keep the convention of
Do you consider accessing props in a change handler is something we would never need? Even though this seems like a good example where this is needed and makes sense

  1. Is there a different way to access props in change handler if I wanted to know which format is selected?
  2. If 1st is a NO then that means that we are getting rid of format prop?

Copy link
Member

@Janpot Janpot Jan 2, 2023

Choose a reason for hiding this comment

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

  1. NO
  2. We don't have ti use the onChangeHandler. We can implement the logic in the component itself, where we have access to the live property values. Similar to how we do onSelectionChange in the DataGrid

regarding onChangeHandler, this is a remnant from a time when we were still very optimistic we would be able build the components library by just taking all the bare MUI components and wrapping each of them with createComponent. In the meantime it has become clear that we needed more customization on most components. Or components that don't exist in MUI at all. And so the onChangeHandler has become a bit obsolete. Maybe we should even think about getting rid of it altogether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can implement the logic in the component itself, where we have access to the live property values

I had that initially done, however using value of datePicker in other places resulted in raw iso string which seemed wrong to me. But sure we can do that

Copy link
Member

Choose a reason for hiding this comment

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

I think we can use the date-only form of ISO8601. This is also unambiguously usable as input for new Date()

@Janpot
Copy link
Member

Janpot commented Jan 3, 2023

Also, this will need to be removed:

Screenshot 2023-01-03 at 11 46 18

@flaviendelangle
Copy link
Member

flaviendelangle commented Jan 4, 2023

We have new pickers that are reaching the beta phase (hopefully in the next 2 weeks).
I would be very interested in feedback from real users.

And more generally, when do you want to migrate to a new major of X ? Will you wait for stable even though Toolpad is experimental, will you update during beta ? Will you allow users to pick a version ?

@Janpot
Copy link
Member

Janpot commented Jan 4, 2023

I'm happy to try out the beta in Toolpad as soon as it comes out.
Upgrading to breaking changes in MUI components is currently an unsolved problem in Toolpad when it comes to custom components. Maybe for now we can still get away with it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Date picker component
5 participants