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

Upgrade date picker #335

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented May 8, 2022

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

@jordisala1991 jordisala1991 changed the base branch from 1.x to 2.x May 8, 2022 19:16
@jordisala1991 jordisala1991 changed the title Feature/upgrade datepicker Upgrade date picker May 8, 2022
@jordisala1991
Copy link
Member Author

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

@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch from cb14cc3 to 3ef3e4a Compare May 8, 2022 19:19
@VincentLanglet
Copy link
Member

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
image
and the fact there are different themes.

@jordisala1991
Copy link
Member Author

It is because tempus dominus is the same, but with a different name. For some reason it got renamed.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch from 7adbd13 to 5b05981 Compare May 26, 2022 06:19
@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch from 7397766 to 445444c Compare May 27, 2022 07:16
@VincentLanglet
Copy link
Member

Will it solve #362 ?

@jordisala1991
Copy link
Member Author

jordisala1991 commented May 27, 2022

Will it solve #362 ?

I don't think so. But well I do not know really

@jordisala1991
Copy link
Member Author

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:

  1. 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)
  2. ...

@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.

@VincentLanglet
Copy link
Member

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 ?

@jordisala1991
Copy link
Member Author

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.

@VincentLanglet
Copy link
Member

This use yes, similar to the previous requirement of jQuery.

With some doc, I think it should be ok

@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch 4 times, most recently from 4cef046 to d24c63d Compare June 3, 2022 07:11
@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch 2 times, most recently from cb0f359 to e34ecb0 Compare June 26, 2022 08:46
@jordisala1991
Copy link
Member Author

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.

@VincentLanglet
Copy link
Member

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.

@jordisala1991
Copy link
Member Author

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)

@VincentLanglet
Copy link
Member

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

@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch from a575808 to aa99a5b Compare June 27, 2022 07:20
@jordisala1991
Copy link
Member Author

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?

@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch from 510e12e to 71cbd57 Compare June 28, 2022 07:02
@VincentLanglet
Copy link
Member

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.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

SonataCI commented Oct 8, 2022

Could you please rebase your PR and fix merge conflicts?

@core23
Copy link
Member

core23 commented Jan 20, 2023

Can't we just deprecate the date picker?

There's a good support for native html date picker: https://caniuse.com/?search=date

@jordisala1991
Copy link
Member Author

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.

@VincentLanglet
Copy link
Member

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.

I don't use sonataAdmin anymore but when I used it we were using the sonata date picker type a lot, even for field which were not in the admin.

You can see with issue like sonata-project/SonataDoctrineORMAdminBundle#1662 that some people prefer date picker over classic date form.

But I can't say how better is the sonata date picker than the default one

@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch 3 times, most recently from 14c41b8 to 80643d6 Compare April 25, 2023 17:17
@jordisala1991 jordisala1991 force-pushed the feature/upgrade-datepicker branch from 80643d6 to 0b782bc Compare April 26, 2023 16:46
@jordisala1991 jordisala1991 removed this from the 2.0 milestone May 7, 2023
@jordisala1991
Copy link
Member Author

Fixed with #427

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

Successfully merging this pull request may close these issues.

4 participants