form-extensions
form-extensions copied to clipboard
sonata-project
Upgrade date picker
Subject
I am targeting this branch, because this breaks BC..
Closes #320
Changelog
### Added
- Added some `Class::newMethod()` to do great stuff.
### Changed
### Deprecated
### Removed
### Fixed
### Security
Not sure if the https://github.com/Eonasdan/tempus-dominus is the best option we have.
It is the major upgrade of our current solution but to me it does not look super up to date...
Other options:
https://github.com/flatpickr/flatpickr
Do you know others? @VincentLanglet
Do you know others? @VincentLanglet
No, but the two you listed seems nice.
About tempus dominus: https://jonathanpeterson.com/posts/state-of-my-picker.html It seems promoted here: https://getdatepicker.com/5-4/
But I kinda like the small flatpickr
and the fact there are different themes.
It is because tempus dominus is the same, but with a different name. For some reason it got renamed.
Will it solve https://github.com/sonata-project/form-extensions/issues/362 ?
With my last changes I have solved:
- Stimulus is no longer bundled here, it is coming from SonataAdminBundle as a global.stimulus variable
- Stimulus application is no longer created here, it is coming from SonataAdminBundle as a global.sonataApplication variable
We have still the benefits of lazy laoded code, so only on the pages that have a date picker , it will load the eonasdan library and the date picker controller. This allows us to define a main application that might load N controllers on SonataAdmin (lazy load if we want them, or not. For example the sidebar navigation could be non lazy load because you need it everywhere) and let other sonataBundles add their own controllers without bundling Stimulus or starting another app themselves.
If we like this strategy we can proceed to implement all the options on the selected datepicker (eonasdan or flatpickr), this is just a proof of concept to see how the whole system could work.
Some things that we might want to address tho:
- What if I am not using Sonata but I want to use form-extensions: We might give you the option of using the datepicker controller on your own app and loading it using stimulus (similar to this: https://github.com/symfony/ux/tree/2.x/src/Chartjs)
- ...
@VincentLanglet wdyt? we are coupling with Stimulus but to me it does not look like jQuery, it is more modern and allows us to do nice things like lazy load and auto bind when new html is created on the fly, without being to heavy.
Another good thing is that the developer could hook on the dispatched event to alter the behavior OR extend our controller and implement its own logic, everything without having to remove the official Sonata javascript. I can provide some example on how this could work too.
What if I am not using Sonata but I want to use form-extensions: We might give you the option of using the datepicker controller on your own app and loading it using stimulus (similar to this: https://github.com/symfony/ux/tree/2.x/src/Chartjs)
This should be still possible indeed. Does it require to use stimulus ?
What if I am not using Sonata but I want to use form-extensions: We might give you the option of using the datepicker controller on your own app and loading it using stimulus (similar to this: https://github.com/symfony/ux/tree/2.x/src/Chartjs)
This should be still possible indeed. Does it require to use stimulus ?
This use yes, similar to the previous requirement of jQuery.
This use yes, similar to the previous requirement of jQuery.
With some doc, I think it should be ok
I know it is not complete, but might be worth if you give a try to see what is done for the moment @VincentLanglet .
Locales only works if you previously set lang on the main html attribute (that will need a PR on admin-bundle, but you can "fake" it by override the base layout template)
What is missing:
- Cleanup the code
- Add the missing supported options
- Convert DateTimes to strings before pass to the view
- Test the controller with jest
- Finish SonataAdmin PR
- Convert the controller to something usable outside Sonta (we might do this later or Next PR, before 2.0 release obv)
It's taking some time to get this right, I don't have much time and I would like to finish with a good solution, to create the foundation for next Sonata Javascript PRs.
I saw you changed to flatpickr. Do you see any implementation differences/interest for one over the other library ?
Looking at https://github.com/sonata-project/form-extensions/pull/335/files#diff-ca4ed843dd8a9065ab8cb9735c6ff7d0e63a973223fa9f5a1d082065c1c7f569, I see that it means that all the options will be different. Did we have the same with tempus-dominus ? I wonder if it's a "too big" BC-break.
I saw you changed to flatpickr. Do you see any implementation differences/interest for one over the other library ?
Looking at https://github.com/sonata-project/form-extensions/pull/335/files#diff-ca4ed843dd8a9065ab8cb9735c6ff7d0e63a973223fa9f5a1d082065c1c7f569, I see that it means that all the options will be different. Did we have the same with tempus-dominus ? I wonder if it's a "too big" BC-break.
With tempus dominus a lot of options changed too.
In fact you have a commit previous to the change to Flatpickr where I finally make the datepicker work with tempus dominus. Some of the options remain the same (like 3-4) and the rest changed somehow.
Good things about tempus dominus:
- Not all options change
- It looks similar to the previous Date picker (improved)
Not so good things about tempus dominus:
- Few translations are available
- It is in beta, and not a lot of activity (one man handling all)
Good things about flatpickr:
- It's size is not a big deal (much fewer kbs of library compared to tempus dominus)
- A lot of translations
- It has some interesting options like ranges in the same date picker (not sure if we can support that tho)
- I think it has a bigger community, and it is stable in its version 4
Bad things about flatpickr:
- The styles do not match with bootstrap a lot (not sure if that is really important)
- All option changes / different features
TBH not sure what option is best, what I am sure is that using Stimulus to load it is the way to go, and that I don't think following tempus dominus is a great idea, we don't know if it will simply die or not.
I personally don't have a lot of 'dp_ options.
I would say it's ok to change to flatpickr but we'll have to write a nice UPGRADE note and try to map the old options => new options, in order to help the developpers.
I personally don't have a lot of
'dp_options.I would say it's ok to change to flatpickr but we'll have to write a nice UPGRADE note and try to map the old options => new options, in order to help the developpers.
Ok, I will try to provide a good upgrade guide, can you try the Pr to see if you like the new date picker?
can you try the Pr to see if you like the new date picker?
I will try to find time (kinda busy atm...)
I need to use both https://github.com/sonata-project/SonataAdminBundle/pull/7838 and https://github.com/sonata-project/form-extensions/pull/335 @jordisala1991 ?
I need to use both https://github.com/sonata-project/SonataAdminBundle/pull/7838 and https://github.com/sonata-project/form-extensions/pull/335 @jordisala1991 ?
Yes
I would love to introduce typescript, but there are things to consider:
good things:
- Better quality of code with typescript (it is typed so its like running phpstan and psalm (but better) on the javascript code)
- Doing it here opens the door to adopt later on AdminBundle 5.0. And not doing it kinda closes the door for admin bundle 5.0.
bad things:
- More changes to do to make everything work.
- More configuration needed to transpile to Javascript (and have to deal with dev-kit different configurations)
So, I would like to do it, but not sure if it is a great idea right now. Maybe we shoul do a two step process. First cleanup javascript changing it to stimulus, and next major change to typescript. wdyt @VincentLanglet ?
The bad thing about that plan is having 2 majors with big bc breaks due to change from older jquery to stimulus and another from javascript to typescript.
So, I would like to do it, but not sure if it is a great idea right now. Maybe we shoul do a two step process. First cleanup javascript changing it to stimulus, and next major change to typescript. wdyt @VincentLanglet ?
Yeah would be better to not delay this release too much
The bad thing about that plan is having 2 majors with big bc breaks due to change from older jquery to stimulus and another from javascript to typescript.
Why is it a BC break ? Even if our code is in Typescript, we should still expose a JS endpoint.
Yes, the normal usage it would still be covered. But if someone wants to compile its own js using our sources it will be a bc break (we might not care for that usage tho)
Yes, the normal usage it would still be covered. But if someone wants to compile its own js using our sources it will be a bc break (we might not care for that usage tho)
I think a minor version with some message in the upgrade note will be enough. We should only cover BC for normal usage
Back to the flatpickr topic, flatpickr allows several options:
$datePickerResolver->setAllowedTypes('defaultDate', ['string', 'string[]', \DateTimeInterface::class, 'DateTimeInterface[]']);
$datePickerResolver->setAllowedTypes('disable', ['string[]', 'DateTimeInterface[]']); // array<{ from: string|Date, to: string|Date }>
$datePickerResolver->setAllowedTypes('enable', ['string[]', 'DateTimeInterface[]']); // array<{ from: string|Date, to: string|Date }>
$datePickerResolver->setAllowedTypes('maxDate', ['string', \DateTimeInterface::class]);
$datePickerResolver->setAllowedTypes('minDate', ['string', \DateTimeInterface::class]);
One questions about it. Should we support both giving strings and DateTimes?
Example:
$form
->add('normal_datepicker', DatePickerType::class, [
'mapped' => false,
'datepicker_options' => [
'disable' => [new \DateTime('tomorrow')],
],
]);
$form
->add('normal_datepicker', DatePickerType::class, [
'mapped' => false,
'datepicker_options' => [
'disable' => ['2022-06-28'],
],
]);
When you pass a string, you must make sure it is in the same format as dateFormat (also another configurable option of flatpickr)
With the current implementation, both options are compatible, but you can't mix datetimes and strings in the same picker, like:
'disable' => ['2022-06-28', new \DateTime('tomorrow')],
So. Should we support strings and DateTimes? and support them at the same time too? or stick with only DateTimes?
One questions about it. Should we support both giving strings and DateTimes? With the current implementation, both options are compatible, but you can't mix datetimes and strings in the same picker, like:
'disable' => ['2022-06-28', new \DateTime('tomorrow')],So. Should we support strings and DateTimes? and support them at the same time too? or stick with only DateTimes?
If it's not hard to do I would say it simpler to support both. But with the current implementation of the Normalizer, it seems like we're supporting mixing them, isn't it ? You're doing a foreach on the value and formatting the Datetime.
Btw, some normalizers could be refactored since they are the same.
Can't we just deprecate the date picker?
There's a good support for native html date picker: https://caniuse.com/?search=date
Wdyt @VincentLanglet ? it would def simplify things for this bundle, it would also remove some feature like the range date pickers.
IMO it would still be nice to add stimulus to SonataAdmin to refactor all the js and simplify a lot of things, but the datepickers can be native.
We can try another Pr to see how it would look with native date pickers and try to solve the range pickers with that.