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

Roadmap to Webpack Encore #7049

Closed
31 tasks done
jordisala1991 opened this issue Apr 13, 2021 · 35 comments
Closed
31 tasks done

Roadmap to Webpack Encore #7049

jordisala1991 opened this issue Apr 13, 2021 · 35 comments
Labels
Milestone

Comments

@jordisala1991
Copy link
Member

jordisala1991 commented Apr 13, 2021

Feature Request

We recently moved to NPM to handle all the dependencies of our code: #7034

First Round

@VincentLanglet
Copy link
Member

It's not clear to me:
If this big to-do-list couldn't be finished (quickly/in time), what is required for a new alpha or the stable version ?

@jordisala1991
Copy link
Member Author

Would be nice for the stable version, but not mandatory. To be noted that some of this will break bc, so if we do it after the stable, will need to be done for 5.0.

For example:

Is it mandatory to have admin lte v3 to release sonata admin v4 stable? Not to me, it would be nice but not mandatory.
If we don't get there in time, it will have to wait for 5.0 (or done in a bc way)

@VincentLanglet
Copy link
Member

VincentLanglet commented Apr 13, 2021

Is it mandatory to have admin lte v3 to release sonata admin v4 stable? Not to me, it would be nice but not mandatory.

If I remember correctly, Admin-lte v3 seems to be a huge change because it will use bootstrap 4 instead of 3.
So all the css written in the admin pages by the developper will need to be rewritten.
There is already a lot of change for the v4. So IMHO it's better to do it for the v5.

I'm ok to release the stable v4 in 1-2 month and to release the v5 at the end of the year.
We should release a new major every year after the v4 to avoid this situation again.

@jordisala1991
Copy link
Member Author

Im okay with not upgrading admin-lte for v4.

This issue is about making changes that bring value and not blocking releases or try to upgrade a lot of things in a single PR.

@jordisala1991
Copy link
Member Author

jordisala1991 commented Apr 15, 2021

We have a problem with this dependency: Eonasdan dateTime picker

It is installed here (previously it was on core-bundle), but the date form types are actually in https://github.com/sonata-project/form-extensions/tree/1.x/src/Type (they were previously on core-bundle too)

We have at least 3 solutions here:

  • Keep Eonasdan here but with the problem that I cannot update it because it requires to change options in: https://github.com/sonata-project/form-extensions/blob/1.x/src/Type/BasePickerType.php (I don't think it is a good idea)
  • Keep Eonasdan here and try to make the form-extensions base types compatible with the old and the new version (not sure if that is possible)
  • Move Eonasdan to the form-extension bridge and modify the resource route (we will still have the update problem, but we can upgrade it for the 2.0 of form-extension)

The only thing with the third option is that we need to configure webpack encore there too.

Edit: To be noted that the eonasdan-bootstrap-datetimepicker is deprecated for tempusdominus-bootstrap-4, but that should be another story because it requires bootstrap 4 and we are not there yet.

wdyt? Maybe there are other options I didn't thought about. @sonata-project/contributors

@haivala
Copy link

haivala commented Apr 15, 2021

I have installed bootsratp 4 for the front of my project with webpack and I'm using "tempusdominus-bootstrap-4": "^5.1.2" with

            ->add('deliveryDate', DatePickerType::class, [
                'required' => false,
                'widget' => 'single_text',
                'html5' => true,
                'format' => 'd.M.y',
                'attr' => [
                    'data-toggle'=>"datetimepicker",
                    'data-target'=>"#castingbundle_actorrequest_deliveryDate"
                ],
                'datepicker_use_button' => false,
                'dp_days_of_week_disabled' => [0,6],
                'dp_min_date'              => $date->format('Y-m-d\TH:i:s'),
                'dp_use_current'             => false,
                'dp_show_today' => true,
                'dp_max_date' => $maxdate->format('Y-m-d\TH:i:s'),
                'dp_pick_date' => true

            ])

and I'm rendering it with {{ form_row(form.deliveryDate) }}
And it works.

I don't know if this is helpful for your problem but I'm just pointing out that it works with the picker type

@jordisala1991
Copy link
Member Author

I have installed bootsratp 4 for the front of my project with webpack and I'm using "tempusdominus-bootstrap-4": "^5.1.2" with

            ->add('deliveryDate', DatePickerType::class, [
                'required' => false,
                'widget' => 'single_text',
                'html5' => true,
                'format' => 'd.M.y',
                'attr' => [
                    'data-toggle'=>"datetimepicker",
                    'data-target'=>"#castingbundle_actorrequest_deliveryDate"
                ],
                'datepicker_use_button' => false,
                'dp_days_of_week_disabled' => [0,6],
                'dp_min_date'              => $date->format('Y-m-d\TH:i:s'),
                'dp_use_current'             => false,
                'dp_show_today' => true,
                'dp_max_date' => $maxdate->format('Y-m-d\TH:i:s'),
                'dp_pick_date' => true

            ])

and I'm rendering it with {{ form_row(form.deliveryDate) }}
And it works.

I don't know if this is helpful for your problem but I'm just pointing out that it works with the picker type

What version of eonasdan-bootstrap-datetimepicker do you use?

@haivala
Copy link

haivala commented Apr 15, 2021

What version of eonasdan-bootstrap-datetimepicker do you use?

none

@jordisala1991
Copy link
Member Author

jordisala1991 commented Apr 15, 2021

Oh you replaced eonasdan-bootstrap-datetimepicker with tempusdominus-bootstrap-4.

Still I don't know if all options of our base date time types are compatible with that package. (Also to be noted that we are still using bootstrap 3 here because of admin-lte v2).

The problem that I am trying to point out is that we are installing a package that gives a features used in another library and that's a problem when trying to upgrade things.

My fear about upgrading this package is this PR: sonata-project/SonataCoreBundle#149

Edit: Now that I see it, most of the changes comes from the fact that it is using bower and replacing a lot of files that are not ours. Adding a third option...

@franmomu
Copy link
Member

can we maybe remove those options from form-extensions and add them from here with a Form Type Extension? like EonasdanDatePickerTypeExtension or something like that.

@jordisala1991
Copy link
Member Author

can we maybe remove those options from form-extensions and add them from here with a Form Type Extension? like EonasdanDatePickerTypeExtension or something like that.

That could be a good thing to try, but not sure about this: https://github.com/sonata-project/form-extensions/blob/1.x/src/Bridge/Symfony/Resources/views/Form/datepicker.html.twig

If those form-extensions types are suposed to work standalone, I am missing something, because there is a coupling with some javascript but the bridge provides none.

@franmomu
Copy link
Member

If those form-extensions types are suposed to work standalone, I am missing something, because there is a coupling with some javascript but the bridge provides none.

oops, I thought the templates were here, in that case I guess we should also override them (and not sure how would look like in form-extensions, maybe they won't exist).

@jordisala1991
Copy link
Member Author

If we remove those templates from the extension, how does it work when used standalone? Not sure about that.

@sonata-project/contributors wdyt?

@jordisala1991
Copy link
Member Author

A lot of the work required is already done or on PRs already opened. I just moved some tasks to second priority

@VincentLanglet
Copy link
Member

A lot of the work required is already done or on PRs already opened. I just moved some tasks to second priority

We might take all the first round in 4.0 and set 5.0 milestone for the second round :)

@wbloszyk
Copy link
Member

A lot of the work required is already done or on PRs already opened. I just moved some tasks to second priority

We might take all the first round in 4.0 and set 5.0 milestone for the second round :)

IMO we should consider integration with webpack encore bundle in 4.0. Then other things from second round can be done in 4.x as experimental another encore build.

@jordisala1991
Copy link
Member Author

I have listed what <script> tags we have in our templates, it will be a lot of work. Maybe not all will be complete for 4.0

@haivala
Copy link

haivala commented Apr 25, 2021

For collection is use @a2lix/symfony-collection in my frontend. Don't know if it'll work for admin.

@jordisala1991
Copy link
Member Author

jordisala1991 commented Apr 25, 2021

For collection is use @a2lix/symfony-collection in my frontend. Don't know if it'll work for admin.

Looks nice to look at how they do it, but IMHO we should not ingrate dependencies that does not look well maintained, with not much activity, not stable release and so on... we could end up with the x-editable drama again.

@haivala
Copy link

haivala commented Apr 25, 2021

For collection is use @a2lix/symfony-collection in my frontend. Don't know if it'll work for admin.

Looks nice to look at how they do it, but IMHO we should not ingrate dependencies that does not look well maintained, with not much activity, not stable release and so on... we could end up with the x-editable drama again.

Sure! Just pointing out. not an actual suggestion.

@jordisala1991
Copy link
Member Author

I didn't found any good plugin to check for license header in stylelint, do you know one?

It should work in a similar way to https://www.npmjs.com/package/eslint-plugin-header but for stylelint. I saw a few, but not very well maintained or not working.

@VincentLanglet
Copy link
Member

@jordisala1991 I just discovered that the recent update of admin-lte in 3.x lost the import of the Source sans pro font.
I assume it's the same for 4.x.

See
https://github.com/sonata-project/SonataAdminBundle/pull/7032/files?file-filters%5B%5D=.css&file-filters%5B%5D=.jpg&file-filters%5B%5D=.js&file-filters%5B%5D=.json&file-filters%5B%5D=.lock&file-filters%5B%5D=.map#
and
ColorlibHQ/AdminLTE@235481d#diff-8f99965cebd561a4920ebfac6065526717f533d00a8ff79b8221c60bb6f549ff

What is the best way for us to add this in 3.x ? And in 4.x ?

@jordisala1991
Copy link
Member Author

jordisala1991 commented Apr 28, 2021

I think adding it at the end of <head> tag should work:

<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic">

If it works, maybe you can add it to our docs?

@VincentLanglet
Copy link
Member

Not sure we have to add something to the doc, if I add it to the standard_layout.
But since there is

       {% block stylesheets %}
            {% for stylesheet in sonata_config.getOption('stylesheets', []) %}
                <link rel="stylesheet" href="{{ asset(stylesheet) }}">
            {% endfor %}
        {% endblock %}

I was wondering if it should be added to the standard_layout template or the option stylesheets ; wdyt ?

@jordisala1991
Copy link
Member Author

Not sure how to handle it, I was thinking on deprecating the default css and javascript configuration now that we have only 1 file each. Maybe it can be on the template with a block to be overriden?

@core23
Copy link
Member

core23 commented Apr 28, 2021

I think adding it at the end of <head> tag should work:

<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic">

If it works, maybe you can add it to our docs?

👎

We should not load ANY third party dependency from an other host. We should load this font via npm or use copy & paste

@haivala
Copy link

haivala commented Apr 28, 2021

Not sure how to handle it, I was thinking on deprecating the default css and javascript configuration now that we have only 1 file each. Maybe it can be on the template with a block to be overriden?

I think there should be way to add simple css or js hacks to admin via config

@VincentLanglet
Copy link
Member

VincentLanglet commented Apr 28, 2021

I think adding it at the end of <head> tag should work:
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic">
If it works, maybe you can add it to our docs?

👎

We should not load ANY third party dependency from an other host. We should load this font via npm or use copy & paste

This was the way it was loaded before in the css

@import url(https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic);

So adding

<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic">

is just loading the font the same way in order to fix the BC-break introduce in 3.x with the admin-lte bump.

In 4.x maybe we could do it here: https://github.com/sonata-project/SonataAdminBundle/blob/master/assets/scss/app.scss ?

Or can we download the font and put them here: https://github.com/sonata-project/SonataAdminBundle/tree/master/src/Resources/public/fonts ?

Not sure how to handle it, I was thinking on deprecating the default css and javascript configuration now that we have only 1 file each. Maybe it can be on the template with a block to be overriden?

I think there should be way to add simple css or js hacks to admin via config

@jordisala1991 wanted to deprecate the default config.
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/DependencyInjection/Configuration.php#L434
Not the extra_stylesheets or extra_javascripts one.

@core23
Copy link
Member

core23 commented Apr 28, 2021

is just loading the font the same way in order to fix the BC-break introduce in 3.x with the admin-lte bump.

For me, this was a security fix and no BR break. I don't want to use any stuff from google and respect the user privacy.

Or can we download the font and put them here: https://github.com/sonata-project/SonataAdminBundle/tree/master/src/Resources/public/fonts ?

This is the way to go 👍

@VincentLanglet
Copy link
Member

VincentLanglet commented Apr 28, 2021

For me, this was a security fix and no BR break. I don't want to use any stuff from google and respect the user privacy.

There was no information in the changelog about the removal, and the design is suffering from this removal.
Plus, admin-lte didn't remove the font but move them to the html instead.
In sonata it was removed from the css but not added to the html. Definitely a BC-break.
If you don't want the font you can still override the admin layout. But you shouldn't remove it for every user of this bundle.

Or can we download the font and put them here: https://github.com/sonata-project/SonataAdminBundle/tree/master/src/Resources/public/fonts ?

I'm not familiar with this.
@jordisala1991 @core23 do you have time to make the PR with the fix ?

@jordisala1991
Copy link
Member Author

IMO 2 options:

  1. Add to the assets, just the static fonts (for 3.x we have to do it this way for sure)
  2. On 4.x we can use this: https://github.com/fontsource/fontsource

@VincentLanglet
Copy link
Member

IMO 2 options:

  1. Add to the assets, just the static fonts (for 3.x we have to do it this way for sure)

Can you do it ? :)

@jordisala1991
Copy link
Member Author

I just did for master: #7131

Once that is approved, the fonts files can be backported to 3.x and the css necessary too. (on the merge back to master will be removed).

@jordisala1991
Copy link
Member Author

I will split the remaining list into smaller issues. The integration with webpack encore is done, now it is time to improve what we have.

@jordisala1991
Copy link
Member Author

There are some issues that keep the work on frontend going:

#7158
#7157
#7156

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

No branches or pull requests

6 participants