-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[3.9] Fix for #11299: Make template.js styling code trigerable by subform row add #19722
Conversation
Moves some code into a global function that is trigerable by subform-repeatable.js. 1. Tooltips 2. Bootstrap button groups 3. Chosen dropdowns
|
||
$('*[rel=tooltip]').tooltip(); | ||
$(root).find('*[rel=tooltip]').tooltip(); |
There was a problem hiding this comment.
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(...);
...
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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 || {};
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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 || {};
You have done exactly what i was talking at #11299, except a very bad thing (see below)
but there is a very bad "hard coding" in this PR
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 ?
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
the template code should not be aware of sub-form field or any other code |
Please don't take me wrong, i support this PR, just this should be moved (aka added by the extension that needs it)
|
yes, and no For me this pull request is good as it is. |
@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. |
i realy don't understand you now, i do not understand why you said the above i will repeat what i said (please read my detailed answer above), the hard-coding i spoke about is
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,
|
@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. |
@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 |
they do not need to know, they do not need to care about it, So they could just call the original arguement (at issue #11299) was
so this discussion is not about what is proper,
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
|
@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. |
no no i did not say not to use events |
Adding Also, jQuery's ready function should simply be written as |
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. |
@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 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. |
@okonomiyaki3000 - Lots to discuss here...
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.
Actually I think that there are two separate events needed here:
So perhaps we should issue a second
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.
Happy to look at simplifying them once we have agreed the above. P.S. On consideration I now agree that |
@okonomiyaki3000 it already done for Joomla 4, see #16016 and #18864 |
This is not bad. The
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. |
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 |
@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. |
@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. |
@Sophist-UK @okonomiyaki3000 just a note here: |
@dgrammatiko Awesome. I always thought that would be the best way. I was also thinking about this: 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? |
@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) |
Pull Request for Issue #11299.
Summary of Changes
Moves some code into a global function that is trigerable by subform-repeatable.js.
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.