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

[3.9] Fix for #11299: Make template.js styling code trigerable by subform row add #19722

Conversation

Sophist-UK
Copy link
Contributor

@Sophist-UK Sophist-UK commented Feb 18, 2018

Pull Request for Issue #11299.

Summary of Changes

Moves some code into a global function that is trigerable by subform-repeatable.js.

  1. Tooltips
  2. Bootstrap button groups
  3. Chosen dropdowns

Testing Instructions

Create a form with a repeatable subform containing a radio control with class="btn-group".

Click to add a new row.

Previous result

Button group on new row would nbot be styled.

New result

Button group is styled.

Moves some code into a global function that is trigerable by subform-repeatable.js.

1. Tooltips
2. Bootstrap button groups
3. Chosen dropdowns
@Sophist-UK Sophist-UK changed the title Make template.js styling code trigerable by subform row add [3.9] Fix for #11299: Make template.js styling code trigerable by subform row add Feb 18, 2018

$('*[rel=tooltip]').tooltip();
$(root).find('*[rel=tooltip]').tooltip();
Copy link
Member

Choose a reason for hiding this comment

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

please cache root variable:

var $root = $(root || document);

$root.find(...);
$root.find(...);
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

(function($)
{
$(document).ready(function()
if (typeof(Joomla) === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

Replace this if statement with:

var Joomla = window.Joomla || {};

Copy link
Contributor Author

@Sophist-UK Sophist-UK Feb 18, 2018

Choose a reason for hiding this comment

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

I took this from cms-uncompressed.js. But happy to change to this if it make sense, and it is a more elegant solution.

(function($)
{
$(document).ready(function()
if (typeof(Joomla) === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

Replace this if statement with:

var Joomla = window.Joomla || {};

@ggppdk
Copy link
Contributor

ggppdk commented Feb 18, 2018

You have done exactly what i was talking at #11299, except a very bad thing (see below)

  1. added a new JS method to contain the template styling
  2. call method on page load for window contents
  3. call method on when a new row is added on the container of the row
  • so there is nothing different about what it was denied a year ago when i had asked for exactly this, just instead of calling it Joomla.applyTemplateStyles you called it Joomla.TemplateJs

but there is a very bad "hard coding" in this PR

  • instead of ... letting the sub-form field contain the registeration of 1 or more events like 'subform-row-add' to call the template JS based styles, etc when the fields needs it !!! you added it inside the template code !

i mean what if the sub-form field later needs the JS-based styles being applied for a different event too, are you going to update the template(s) again and its overrides too ?

  • you have added a hard-coded knowledge of the existense of 'sub-form' field inside the template

can you also add 13 more hardcoding for 13 other extensions that need to call template styles when they want to add some contents into the DOM ? (i have a few)

At least if my suggestion of standarizing the method was accepted then

  • sub-form field and every custom code can call it when needed
  • they will simply check if methods exists and call it !

the template code should not be aware of sub-form field or any other code
this is a really bad hard coding

@ggppdk
Copy link
Contributor

ggppdk commented Feb 18, 2018

Please don't take me wrong, i support this PR,
i hope it is accepted

just this should be moved (aka added by the extension that needs it)
and method should be checked if it exists

$(document).on('subform-row-add', function(event, row) { Joomla.TemplateJs(row); });

@Fedik
Copy link
Member

Fedik commented Feb 19, 2018

the template code should not be aware of sub-form field or any other code
this is a really bad hard coding

yes, and no
for Joomla! 3 it only one possible solution
for Joomla! 4 we have an evens which allow to do this without "hardcoding" see #18864 #16016

For me this pull request is good as it is.
Do not need to move anything, each extension should care about their script on its own, we just provide an event that allow to do it.

@Sophist-UK
Copy link
Contributor Author

@ggppdk Using events is NOT "hard coding" - it is loose coupling. So instead of subform-repeatable having to know exactly what other components needs to know about a row being added and calling them explicitly, instead it just tells the world that it has added a row and lets every other piece of code that is interested listen for the event and handle it.

This is (IMO) a much better way of constructing large complex pieces of code.

@ggppdk
Copy link
Contributor

ggppdk commented Feb 19, 2018

Using events is NOT "hard coding" - it is loose coupling. So instead of subform-repeatable having to know exactly what other components needs to know about a row being added and calling them explicitly

i realy don't understand you now, i do not understand why you said the above
i said nothing about the above, nor did i imply it

i will repeat what i said (please read my detailed answer above), the hard-coding i spoke about is

  • that template needs to know of an ABC extension / plugin, by knowing the event(s) that the that extension has

the above is a really silly hard-coding

the only argument that i could understand about doing it is what Fedik said (implied ?) that in J4 there is a more proper solution,

  • but still this argument about J3 and J4 would make sense if J3 was at its very end of life, only then

@C-Lodder
Copy link
Member

@Sophist-UK basically it's knowing the difference between a template and extension. As stated before, this is a fair fix, however we should not be adding code to the template Javascript file to cater for an extension. The template dictates the design of a site, nothing else. In other words, is there is some Javascript that is needed to fix the subform fields, it should reside in the subform fields Javascript files.

@Sophist-UK
Copy link
Contributor Author

@C-Lodder That makes no sense, particularly for this example.

The subform-repeatable (or indeed 3rd party extensions) cannot know for example that a template uses Chosen to enhance dropdowns. That is a template decision, and subform-repeatable should not need to know about it. But the template needs to recognise that on modern web sites not all HTML arrives at page load time - some comes from Ajax calls - so if the template uses e.g. Chosen, then it needs to provide a way to have it reapplied to the dropdowns by subform-repeatable or any extension that dynamically updates the HTML.

However I agree that subform.row.add is the wrong approach because it is too specific and the template is tied to that functionality, and joomla:updated is better because it is more generalised and can be dispatched by any extension that needs to reapply template styling. (I would personally have used something like joomla:htmlUpdated or joomla:applyTemplateStyle or something a bit more descriptive, but the concept still holds true.)

@ggppdk
Copy link
Contributor

ggppdk commented Feb 19, 2018

The subform-repeatable (or indeed 3rd party extensions) cannot know for example that a template uses Chosen to enhance dropdowns. ...

they do not need to know, they do not need to care about it,
they would only need to know if there is a way to apply template styles

So they could just call Joomla.TemplateJs(container) (if it exists aka implemented by template) whenever they decide they need to
by registering their own custom events (whatever their names are) that make use of the method

the original arguement (at issue #11299) was

  • not to STANDARIZE a new JS API method for Joomla templates since J4 is coming

so this discussion is not about what is proper,
we know that proper thing , is to avoid hard-coding for something that is

  • neither critical
  • nor it is 3rd-party code that we have no direct control over changing its code

aka then the above 2 things would justify the trade-off of doing this

Finally since this PR adds the "non-standarize" method of 'Joomla.TemplateJs' , i still fail to see the need for this hard-coding

Anyway this hard-coding will not effect 3rd party extensions, so lets just test and merge it

  • my extension code will just check if Joomla.TemplateJS() exists and prefer using it instead of falling back to some guessing of what the template styles are !

@Sophist-UK
Copy link
Contributor Author

@ggppdk I am not going to argue about this further - you want to see tight coupling by direct calls, I (and the Joomla leadership - lots of examples) believe that loose coupling, in this case using events, is preferable.

@ggppdk
Copy link
Contributor

ggppdk commented Feb 19, 2018

  • you want to see tight coupling by direct calls

no no i did not say not to use events
nor anything related to "tight coupling" thing

@okonomiyaki3000
Copy link
Contributor

okonomiyaki3000 commented Apr 24, 2018

Adding Joomla.TemplateJs to the Joomla namespace is a big mistake. This function will never need to be called outside of this specific context but, once it has been created there, removing it in the future will be considered a BC break because (in theory) some other extension could use it (even though nobody ever will).

Also, jQuery's ready function should simply be written as jQuery(function($) { });, this jQuery(document).ready(function () {}); style is really unnecessary.

@Sophist-UK
Copy link
Contributor Author

The purpose of adding Joomla.TemplateJs to the Joomla namespace has been done deliberately because I believe other extensions needs this functionality.

For example when you use a template that uses Chosen dropdowns, and an extension dynamically loads HTML, then it needs to initialise Chosen on the new dropdowns.

This is exactly the purpose of this PR - to make a standard way for extensions to initialise code to match template styling.

@okonomiyaki3000
Copy link
Contributor

okonomiyaki3000 commented Apr 24, 2018

@Sophist-UK I see what you're trying to do but I don't think the template is the right place to do it. Not every template will have this function so it's not like any extension that loads html can just call it and expect everything to be fine. On top of that, this function doesn't even do everything that may need to be done. It handles chosen, tooltips, and button groups. There are any number of items that may require initialization that this doesn't cover.

The subform-row-add event was added to so that anything that needs initialization can do it automatically. The only problem with it is that it is tied to subform. We should have called it 'html-dom-update' or something like that. Then anything that updates the dom can trigger it and anything that needs initialization can listen for it.

Plus, I'm sorry to say, you're adding a bit of complexity to these javascript files when they are already more complex than they need to be. They can actually be simplified and accomplish the same thing.

@Sophist-UK
Copy link
Contributor Author

Sophist-UK commented Apr 24, 2018

@okonomiyaki3000 - Lots to discuss here...

Not every template will have this function so it's not like any extension that loads html can just call it and expect everything to be fine.

True - so if we use this approach then I guess there needs to be a default function that is overridden by a template - and that sounds messy. So it is probably better to use an event listener instead.

The subform-row-add event was added to so that anything that needs initialization can do it automatically. The only problem with it is that it is tied to subform. We should have called it html-dom-update or something like that. Then anything that updates the dom can trigger it and anything that needs initialization can listen for it.

Actually I think that there are two separate events needed here:

  1. One for things you need to do because the new subform row contains new HTML - this is the same code you need to run whenever there is new HTML - this event could be called html-dom-updated as you suggest.,
  2. A second for things you need to do specifically because it is a subform row that has been added - who knows what that might be, but as a simple example might be updating a <div> containing a counter of the number of rows.

So perhaps we should issue a second html-dom-updated event immediately before the subform-row-add event. I don't think this will introduce any incompatibility because no one currently uses the new event and they can switch at their leisure.

On top of that, this function doesn't even do everything that may need to be done. It handles chosen, tooltips, and button groups. There are any number of items that may require initialization that this doesn't cover.

All I am doing is adding what I know needs adding. If there are others then either point them out to me and I will do them, or others can add them when they find they are missing.

Plus, I'm sorry to say, you're adding a bit of complexity to these javascript files when they are already more complex than they need to be. They can actually be simplified and accomplish the same thing.

Happy to look at simplifying them once we have agreed the above.

P.S. On consideration I now agree that Joomla.TemplateJs was indeed "a big mistake". Thanks for being patient with me whilst I came to realise this. :-)

@Fedik
Copy link
Member

Fedik commented Apr 24, 2018

The subform-row-add event was added to so that anything that needs initialization can do it automatically. The only problem with it is that it is tied to subform. We should have called it 'html-dom-update' or something like that. Then anything that updates the dom can trigger it and anything that needs initialization can listen for it.

@okonomiyaki3000 it already done for Joomla 4, see #16016 and #18864

@okonomiyaki3000
Copy link
Contributor

So perhaps we should issue a second html-dom-updated event immediately before the subform-row-add event. I don't think this will introduce any incompatibility because no one currently uses the new event and they can switch at their leisure.

This is not bad. The html-dom-updated event object could also contain some kind of information about exactly what type of thing was being updated but then, how would such a thing even be identified? So maybe having two separate events is the best way. How about if you have two subforms on the same page? Do we need a way to differentiate them so that we know which one the events came from? I guess this is already not hard to find out if we really care to know.

All I am doing is adding what I know needs adding. If there are others then either point them out to me and I will do them, or others can add them when they find they are missing.

That's just it: there is not a finite list of things. It can depend on any number of factors. A centralized solution can't really work.

@Sophist-UK
Copy link
Contributor Author

Closed in favour of PRs #20221 / #20222 / #20224 / #20225.

@okonomiyaki3000 Actually there are two finite lists of things. The template needs to hook into the html-dom-updated event to implement the finite list of things it uses, and then extensions also need to hook into both events to implement the finite list of things they know about.

@Sophist-UK Sophist-UK closed this Apr 25, 2018
@okonomiyaki3000
Copy link
Contributor

@Sophist-UK Yes, the template can support a finite list of things and each individual extension can support a finite list of things. What I'm saying is that the list of all possible things that may need to be supported is not finite.

@Sophist-UK
Copy link
Contributor Author

@okonomiyaki3000 If we are going to be literal about it, the list is definitely finite just unknown.

My point is that the different authors of the various templates and extensions know what their own code needs, so we need event infrastructure that enables them to easily write it and hook in.

@dgrammatiko
Copy link
Contributor

@Sophist-UK @okonomiyaki3000 just a note here:
as @Fedik already mentioned in J4 we have proper event system for the subform but even better we're implementing all joomla fields as Custom Elements and thus we are actually utilising the native browser (DOM) events for the subforms. Since custom elements are ES6 classes that have a constructor and lifecycle events like connectedCallback and disconnectedCallback we don't need to fire our own Joomla-whatever or as @Sophist-UK mentioned the html-dom-updated events. We use the basic connectedCallback which is essentially the same without writing any extra lines of code! WIN

@okonomiyaki3000
Copy link
Contributor

@dgrammatiko Awesome. I always thought that would be the best way. I was also thinking about this:
https://stackoverflow.com/questions/3219758/detect-changes-in-the-dom#14570614

Which it seems we will not need in J4 and which might not be suitable for J3... Doesn't work below IE9. Is J3 expected to support older browsers?

@dgrammatiko
Copy link
Contributor

@okonomiyaki3000 there is a promise that J3 should be IE8 compatible. But mutation observer is polyfillable for IE8 and we already have a JHtml method to add polyfills (eg: https://github.com/megawac/MutationObserver.js/tree/master)

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

Successfully merging this pull request may close these issues.

None yet

7 participants