joomla-projects / joomla-es6

Joomla Javascript ES6 Repository
GNU General Public License v2.0
4 stars 5 forks source link

Remove inline script of the type field and transform to es6 #98

Closed laoneo closed 6 years ago

laoneo commented 6 years ago

Pull request for issue #55.

Test: Create a new custom field and change the type before save. The page should reload.

@dgt41 should I use the CE for the loading layer?

C-Lodder commented 6 years ago

@laoneo already done here: https://github.com/joomla/joomla-cms/pull/19677

@dgt41 did you want to move that PR to this repo?

laoneo commented 6 years ago

@C-Lodder it was an open todo in the issue #55. You mean the CE or the inline script and es6 migration which was done already?

dneukirchen commented 6 years ago

@C-Lodder @dgt41 @laoneo what do you want to do with this? As i wrote in multiple places i prefer to have stuff like this in a script instead of a custom element. We need to make a decision about this.

C-Lodder commented 6 years ago

Having the loader icon as a custom element is perfectly fine imo

dgrammatiko commented 6 years ago

I still prefer custom elements everywhere for few reasons:

dgrammatiko commented 6 years ago

Another aspect that I think we re currently doing wrong (and probably we should fix it ASAP) is the extensive use of DOMContentLoaded event. Let’s convert those to joomla loaded

C-Lodder commented 6 years ago

Dimitris, any reason why we should replace a native event listener with our own?

dgrammatiko commented 6 years ago

Then we can lazy load the scripts, if DOMContentLoaded is used then we have to have the scripts in the document before page is loaded. It's about flexibility and also start using our API everywhere

C-Lodder commented 6 years ago

Fair play. Seems logical

laoneo commented 6 years ago

Should we close this one in favor of https://github.com/joomla/joomla-cms/pull/19671 ?

dneukirchen commented 6 years ago

Having the loader icon as a custom element is perfectly fine imo

Fully Agree, a custom element only for the loader is perfectly fine, but heaving a custom element for a custom field type that depends on multiple external DOM elements and also controller code (reload) (like suggested in joomla/joomla-cms#19671) is what im complaining about.

I still prefer custom elements everywhere for few reasons:

  • we deliver es6 code to supported browsers
  • we lazyload that code

Should we close this one in favor of joomla/joomla-cms#19671 ?

For me this PR is still valid and makes more sense than a custom element, because its just binding a change event to a listener (we dont need custom elements and polyfills for this).

This needs a decision from @dgt41

dgrammatiko commented 6 years ago

Well one way or another the category field will be custom element at the end because, apart from the obvious coupling here with the custom fields it needs to fulfill 2 more things: a search function and a create new category. So getting couple of lines for the already existing coupling to custom fields is neither weird or awkward. There is a very strong bond there and the custom element is just showcasing it.

dgrammatiko commented 6 years ago

i dont see client side lazy loading in joomla 4.0

@dneukirchen it's there you need to check, eg this is what the server is sending to the browser:

screen shot 2018-03-10 at 11 08 24

(there is no joomla-alert.js there)

and this is the DOM after parsing/executing:

screen shot 2018-03-10 at 11 08 08
dneukirchen commented 6 years ago

Regarding client side lazy loading: Im not saying that it is not possible to lazy load, but we still have some problems with lazy loading (media overrides, 3dp usage, circular deps, multiple http requests for same resource, dependency discovery, no build tool...) so there is a lot of work to do and joomla is not a javascript heavy app so im not sure if its worth it. We now deliver es6 files, so thats a good step in the right direction. Lets first finish this and then we can discuss lazy loading as core feature. (might be also a good candidate for 3dp plugin + template).

Regarding custom elements... i think i made my point clear. Im still convinced that we should not use custom elements as application-scripts. In your example: having a custom element for the loading indicator and one for a pretty-select (with search) is absolutely fine for me. But an app/component script should define how they play together and how they interact with external DOM elements and backend code. Not the custom element itself.

dgrammatiko commented 6 years ago

Lets first finish this and then we can discuss lazy loading as core feature

Actually for webcomponents this is already implemented. So using custom elements everywhere means lasyloading js (even ES6, for which we don't have a solid solution) right now!

But an app/component script should define how they play together

There is no such limitation regarding the custom elements themselves. Also as I said before this is an element that represent some client side functionality of com_fields, so it's natural to have bonds to the backend. If there was a better approach in the backend for this then we wouldn't have this discussion. The custom element feels too attached to the DOM due to the implementation of custom fields and honestly since all these DOM elements (input[name=task]) is a hard requirement for the custom fields to work properly then the dependency in the custom element is natural

dneukirchen commented 6 years ago

Actually for webcomponents this is already implemented. So using custom elements everywhere means lasyloading js (even ES6, for which we don't have a solid solution) right now!

Yes, but the problems i mentioned in my previous comment (media overrides, 3dp usage, circular deps, multiple http requests for same resource, dependency discovery, no build tool...) are not solved.

Disadvantages:

Risks:

So what is the reason that justifies the usage of custom elements here?

dgrammatiko commented 6 years ago

Custom elements are part of the DOM so seriously I don’t get what you mean with this. Also even if you have it as simple js script you still end up with the same exactly dependency so what are we even talking about?

About those possible problems with custom elements: all of them are solved already

dneukirchen commented 6 years ago

Custom elements are part of the DOM so seriously I don’t get what you mean with this

Tightly coupled to existence of external DOM elements and backend code. Also more code and need of polyfills.

dgrammatiko commented 6 years ago

That’s due to the way this field is build. You end up with exactly the same dependencies in the plain js script, so???

dneukirchen commented 6 years ago

No, we need more code and polyfills.

So what is the reason that justifies the usage of custom elements here?

dgrammatiko commented 6 years ago

What polyfill? These are already loaded due to joomla-alerts which exist in every template. Also polyfills only for edge and ie next version of ff comes with web components and till j4 hits the road polyfill will be required only for ie. also there is no extra code really, as the extra lines are there to elevate the browsers event system instead of DOMContentLoaded which by the way makes it impossible to lady load the script...

dgrammatiko commented 6 years ago

By the way lazy loading is a requirement for PWA

C-Lodder commented 6 years ago

@dneukirchen Polyfill are only for crap browsers and FF. I think FF support custom elements with some options changed in about:config though

dgrammatiko commented 6 years ago

Nightly already supports custom elements Shadow DOM etc

dneukirchen commented 6 years ago

Yes, i know... but still: this is only an onChange listener which reloads the page... Without custom element this is ~4 loc. No need for polyfills (in ALL browsers). No need for browser/feature detection. No need for different build/test tools. No need for a custom element which is tightly coupled to external DOM or backend code.

Im asking again:

So what is the reason that justifies the usage of custom elements here?

dgrammatiko commented 6 years ago

8a26e741-ee0d-44ff-8c8d-af4077bf032d

Those 4 loc cannot be lazy loaded tho...

dgrammatiko commented 6 years ago

@dneukirchen I wouldn’t be so stubborn on this but seriously I want all the fields to become ce no matter what’s the logic, their weight or their deps.

dneukirchen commented 6 years ago

Those 4 loc cannot be lazy loaded tho...

Why should it not be possible to lazy load a script.

But i give up... I still cant find a good reason why we should (incorrectly) use custom elements for those 4 liners.