Skip to content

Comments

Fix subform chosen#12993

Merged
zero-24 merged 3 commits intojoomla:stagingfrom
okonomiyaki3000:fix-subform-chosen
Dec 21, 2016
Merged

Fix subform chosen#12993
zero-24 merged 3 commits intojoomla:stagingfrom
okonomiyaki3000:fix-subform-chosen

Conversation

@okonomiyaki3000
Copy link
Contributor

@okonomiyaki3000 okonomiyaki3000 commented Nov 24, 2016

Pull Request for Issue # .

Summary of Changes

When adding a new row in a subform that contains one or more selects that should be turned into 'chosen', chosen initialization will be run on the new row's elements.

This was originally part of #12542 but separating it to keep PRs small and simple.

Testing Instructions

Add new rows to any subform that contains 'chosen'. Before this change, they should remain as regular selects. After, they will be proper chosens.

Documentation Changes Required

No

@ggppdk
Copy link
Contributor

ggppdk commented Nov 24, 2016

Nice to see these PRs to apply things like styling and behaviour on a specific container

function JSfunction(..., container)
{
	container = container || document;
	$(container).find('.classname')....
	....
}

i had suggested this in the past, but i got a negative answer, i do not remember why,
i think it was to wait for some new API in J4 ?

now with the subform, the need for this is more obvious

  • also another reason is to apply it, to AJAX retrieved HTML

currently i am using my own API to do such things

@okonomiyaki3000
Copy link
Contributor Author

@ggppdk The reason this is able to work is that subform is emitting subform-row-add whenever a row is added. It's nice but a couple of things could be done to generalize this better.

First, Joomla could set a listeners at the document level the react to subform-row-add and other, similar dom manipulation events (which don't currently exist and would need to be manually triggered by whatever process is doing something with the dom). Then, Joomla could trigger some new universal dom manipulation event (something like JDOMNodeInserted) and pass it all the relevant information. Then, chosen and others use that one event rather than listening for subform-row-add plus whatever others might get added in the future. In fact, if there were such JDOM events, subform and others that manipulate the dom could just trigger them directly anyway.

Of course, we'd rather be using Mutation Observers but that's just not something we can realistically rely on right now. Or maybe we can but provide something like a polyfill for older browsers?

Another thing that might be nice would be to have a new Joomla core js function like Joomla.registerInitializer which could be passed a function which takes a container as its argument and performs some kind of initializations within that container. Then, Joomla will call this function once when the dom is ready (or immediately if the dom is already ready) by passing document and again whenever a node is added to the dom by passing the node itself. Then each of these init functions could be simplified a little bit and redundant code could be dropped.

@okonomiyaki3000
Copy link
Contributor Author

Actually, now that I think about it, a function like registerInitializer, if generalized even further, could go a long way toward decoupling Joomla from any js framework. As it is, most js you need to add to a page will put some initialization code in a jQuery ready function like:

jQuery(function ($) { /* init code */ });

But if Joomla had its own ready function, we could just do it like this:

Joomla(function () { /* init code */});

Then, typically, any functions you pass will just get handled by the jQuery ready event anyway but that could be replaced as needed. Additionally, the init function you pass could be called when changes are made to the dom. We'd probably want to specify this behavior with a flag or something. Maybe like:

Joomla(function () { /* init code to be called on domready and node insert */}, true);

The function should expect to be passed either document or a dom node so that it knows the scope that it's meant to operate on.

I haven't given it much thought yet but it seems like a good idea.

@Fedik
Copy link
Member

Fedik commented Nov 25, 2016

@ggppdk there was couple suggestion already, one of them #6357,
and found one even older #1260 from 2013 😄

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Nov 26, 2016

But if Joomla had its own ready function, we could just do it like this:

Please don't do that. ECMAScript 6 is widely supported from every html5 browser and there is:
document.addEventListener('DOMContentLoaded' function(){})
and
document.createEvent(type);
No need for jQuery or any other such library. Let's move on and away from all these messy and useless libraries.

@phproberto
Copy link
Contributor

document.addEventListener('DOMContentLoaded' function(){})

Remember that Joomla still supports IE8

@dgrammatiko
Copy link
Contributor

@Fedik
Copy link
Member

Fedik commented Nov 26, 2016

I have tested this item ✅ successfully on 3c29533


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12993.

@okonomiyaki3000
Copy link
Contributor Author

@DGT41 Vanilla JS is just fine but, without mutation observers, it doesn't solve the problem of dom updates.

@dgrammatiko
Copy link
Contributor

@okonomiyaki3000 you can use a polyfill for mutation observer: https://github.com/megawac/MutationObserver.js.
and therefore use it safely even for IE8. My personal opinion (and that's all there is here) is to stick to javascript standards as much as possible. This might come with a slight cost for 3.x (polyfills) but will be a cleaner base for J4 (>IE11).

@brianteeman
Copy link
Contributor

brianteeman commented Nov 28, 2016 via email

@dgrammatiko
Copy link
Contributor

@brianteeman we already introduced polyfills in other instances (e.g. tinyMCE) so that shouldn't be a problem. Also I am aware of the EOL for 3.x and I don't see a possible conflict here, but maybe I'm missing something...

@okonomiyaki3000
Copy link
Contributor Author

You'd still end up with every bit of init code needing to set up a handler for DOMContentLoaded and then also add some mutation observer code. This gets pretty redundant when you have chosen, tooltips, calendar, etc all on the same page and all setting these things up. That's why I think it's not bad for Joomla to provide a function that takes your init function and applies it whenever needed.

@laoneo
Copy link
Member

laoneo commented Dec 20, 2016

I have tested this item ✅ successfully on 3c29533


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12993.

@jeckodevelopment
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12993.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 20, 2016
@zero-24
Copy link
Contributor

zero-24 commented Dec 21, 2016

@DGT41 can you please confirm that the problem is fixed in the final stage? I'm not sure at this stat of the PR that the issue you discussed is finaly fiexed now?

@dgrammatiko
Copy link
Contributor

@zero-24 this PR is fine as is, go ahead and merge it!

@zero-24 zero-24 merged commit 0158cdb into joomla:staging Dec 21, 2016
@joomla-cms-bot joomla-cms-bot added Unit/System Tests and removed RTC This Pull Request is Ready To Commit labels Dec 21, 2016
@zero-24
Copy link
Contributor

zero-24 commented Dec 21, 2016

Done thanks 👍

@zero-24 zero-24 added this to the Joomla 3.7.0 milestone Dec 21, 2016
@bembelimen
Copy link
Contributor

Hello,

I really do like the idea and that chosen within the subforms are resolved, but I also really don't like the idea of a depending event in chosen just for subforms.

It should be the other way arround:

Chosen should offer a function ($.fn.chosen e.g.) which can be used by other javascripts and then THEY can add events etc.

@dgrammatiko
Copy link
Contributor

@bembelimen actually joomla should move away from chosen. Reason well explained here: harvesthq/chosen#1740 (comment)

@bembelimen
Copy link
Contributor

On long term, I fully agree @DGT41 on short term perhaps we should change the Joomla! behavior?

@okonomiyaki3000
Copy link
Contributor Author

okonomiyaki3000 commented Dec 22, 2016

@bembelimen No that is not the way it should be. In that scenario, each and every control type that needs some kind of initialization in javascript (there are many) would need to provide such a function and subform would need to be aware of all of them. This is barely maintainable for the standard field types but it breaks down completely when you consider that it's possible to write custom field types.

You are right that the current approach is not perfect but look at it like this.
Now - Any field that requires initialization must be aware of subform (slightly troublesome and doesn't allow for other fields to manipulate the dom like subform does).
Yours - Subform must be aware of any field that requires initialization (unmanageable).
Better - Joomla provides a kind of notification system which dom-mutating functions could trigger events and fields requiring initialization could listen for them (mostly fine).
Best - Use mutation observers so that dom mutations don't need to be explicitly triggered.
Besterer? - Could it be possible to do custom field types as web components and would that mean that their initialization steps might be done implicitly?

So we should look at other possibilities going forward but this is good for now.

@okonomiyaki3000 okonomiyaki3000 deleted the fix-subform-chosen branch December 22, 2016 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.