Failure

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
19 Sep 2013

previous pull #1302
I made some updates based on @Bakual idea about apply chosen only for .advancedSelect
so here I removed JHtml::_('formbehavior.chosen', 'select'); from all tmpl/, and added options to the template for allow to enable/disable JHtml::_('formbehavior.chosen'); ... that will work only for <select> with class .advancedSelect

and I made new pull because previous have conflicts....

Links:

avatar Fedik Fedik - open - 19 Sep 2013
avatar mbabker
mbabker - comment - 19 Sep 2013

Please restore the JHtml::_('formbehavior.chosen', 'select'); calls in the front end component views. Those should be handled in a separate patch if you intend on changing Protostar's behavior.

avatar Fedik
Fedik - comment - 19 Sep 2013

ah, yes, you are right ... I missed this

avatar Bakual
Bakual - comment - 19 Sep 2013

This one will essentially remove chosen from the whole backend as currently, the class "advancedSelect" isn't used anywhere. If you're going to change the call from JHtml::_('formbehavior.chosen', 'select'); to JHtml::_('formbehavior.chosen'); (which I support!) you need to add the advancedSelect class to each select which should be supported by chosen (currently that's each select present). Otherwise you could as well just remove chosen alltogether.

avatar Fedik
Fedik - comment - 19 Sep 2013

@Bakual that was the first part of the plan, delete all :)
add advancedSelect it other part of the plan

avatar Bakual
Bakual - comment - 19 Sep 2013

@Fedik The second part is by far the harder part ;-)

What about having two parameters then, one for back- and one for front-end. This issue isn't necessarily exclusive to the back-end as far as I know.

@betweenbrain Since he did it as a template parameter, the frontend would have to be taken care by the frontend template. But here it would be a backward compatibility break for all existing templates which would have to be updated to include the chosen call if they wish to support it. Realistically speaking I don't think that is going to be accepted for the frontend.

avatar Fedik
Fedik - comment - 19 Sep 2013

I think "Permission Settings" can live without the Chosen, cause there 100 selects in each...

avatar Fedik
Fedik - comment - 19 Sep 2013

small question: how to add the class to JHtmlSidebar::addFilter? ( example ) , not sure that good idea to do it in view.html

avatar Bakual
Bakual - comment - 19 Sep 2013

For sake of consistency between the options tabs it should be there as well imho. But it's a design question and I'm not a designer :)

From a first look you will also have to threat the XML files for the menu items in each frontend views. Those are used in the menu manager.
In #1620 I made it the "lazy" way by adding advancedSelect as default class to the list formfield. As far as I remember those files didn't have any classes in it, so the default class from the formfield would be enough for those.

Also some selects will be hidden in a model or sidebar layouts and stuff like this. Keep em coming :-)

avatar Bakual
Bakual - comment - 19 Sep 2013

For the JHtmlSidebar you need to look at the layout in layouts\joomla\sidebars\submenu.php

avatar Fedik
Fedik - comment - 19 Sep 2013

performance is the main reason for skip it for "Permission Settings", I noticed that the "Global configuration" page (for example) works a lot faster if disable Chosen just for "Permission Settings" ;)

and thanks for tip about sidebars

avatar Bakual
Bakual - comment - 19 Sep 2013

It will be faster for sure, but isn't the purpose of this PR to let the user decide if he wants chosen or not? :)

avatar Fedik
Fedik - comment - 19 Sep 2013

the purpose make back end faster through disable the chosen ;)

avatar Bakual
Bakual - comment - 19 Sep 2013

Then there is no reason to leave the permission tab out. Support chosen there as well and those who want a fast backend turn chosen off globally.

avatar Fedik
Fedik - comment - 20 Sep 2013

ok, done I hope, who want to try? :-p
only the fields componentlayout and modulelayout do not takes the class from XML ... looks like bug

also I added advancedSelect for "Permission Settings" (field rules) ... but I still think that it is a very bad idea, because performance ;)
so maybe better remove?

avatar Bakual
Bakual - comment - 20 Sep 2013

Performance is taken care of by the template parameter. So that's not an issue to take of anymore. Also it depends a lot on the client (computer, browser, user) if it even is an issue or not. So leave it in and let the user turn it off if he doesn't like it :-)

The componentlayout and modulelayout are generated by the respective formfield found in /libraries/legacy/form/componentlayout.php and modulelayout.php. I think you should be able to add the advancedSelect class there.

I will be away the whole weekend (vacation and JoomlaDay), but I'm happy to test it next week.

avatar Fedik
Fedik - comment - 20 Sep 2013

maybe better I do new pull for fix componentlayout and modulelayout ... should be not difficult, will see...

avatar Bakual
Bakual - comment - 20 Sep 2013

Make ut so it defaults to the class advancedSelect. Then it will also work for 3rd party extensions without them needing to change all files. Same for the list field like I did in my PR.
I think the doc says anyway that it defaults to a class.

avatar Fedik
Fedik - comment - 20 Sep 2013

I already added advancedSelect for all these fields in XML`s that I found, so think small check whether the class exist will be enough #2027

avatar Bakual
Bakual - comment - 20 Sep 2013

You know, it also has to work for all 3rd party extensions. Adding a default class would help a lot here. Without it we developers need to update all of our XML files only to get them support chosen again. Not a nice move if it could be done with a default class instead :)

avatar Fedik
Fedik - comment - 20 Sep 2013

ok, added checks (#2027) , if no class then will be used advancedSelect :)

avatar phproberto
phproberto - comment - 20 Sep 2013

I don't like to add a class in the selectors at all. That means that the selectors are describing the way that the field is going to be shown in the template which is wrong IMO.

If you want to add a class for the fields you shoudl add a class descriptive of the field itself like field-modulelayout or whatever. Then in the JS of the template you can do something like:

$('.field-modulelayout', '.field-componentlayout').chosen();

IMO the function should be defined as:

public static function chosen($selector = '.chosen', $debug = null)

That describes that is going to use chosen so anyone in a template fastly know how to use it. And then we also can create select2 field to let them to decide what to use. I already have implemented the select2 field so I can contribute it if people likes it. BTW I did the test in the config and it's still slow with select2. So the fix passes to remove chosen or select2.

Doing this is:

JHtml::_('formbehavior.chosen', 'select');

is something people can do to in templates. So in my template I can use:

JHtml::_('jquery.select2', 'select');

In my opinion the fix for config passes by removing the chosen call ATM.

In the future ideally we can do one more to fix this kind of problems of CSS classes being added in the XML:

  • We allow to override for XML files
  • We ensure that any JS, CSS & HTML code loaded in the core can be overriden using JLayout for field rendering i.e.

If you want to add an option in the template to decide if they want to load chosen the code should be there so you should add the if statements in each view or just do the JS call as I showed you before.

avatar Bakual
Bakual - comment - 20 Sep 2013

Actually the class 'advancedSelect' is better than a class 'chosen' because it is not related to a specific implementation. It could be chosen or select2 or whatever. And it is the default class for the chosen function in Joomla since its introduction. So you can't change that now.

avatar phproberto
phproberto - comment - 20 Sep 2013

chosen and select2 have different options (which remembers me that the core chosen implementation is wrong because it's missing the options) so you have to change the class anyway if you decide to swtich. In fact it can be more confusing for people to use something generic rather than something that specifically describes what is going to happen.

For example in 3.5 you want to switch to select2 and then people have to change/check the options passed instead of directly search the chosen call and replace it with the select2 call.

I agree that change it now is maybe not possible but it doesn't make it the right.

avatar mbabker
mbabker - comment - 20 Sep 2013

The PHP method being invoked is pretty specific about what is being called (Chosen), the class name being generic isn't an issue IMO. And I actually think advancedSelect for the class name works out well; you're specifying that you're using some sort of JS to enhance the basic <select> element.

avatar phproberto
phproberto - comment - 20 Sep 2013

To see what I'm talking about in administrator/components/com_config/views/application/tmpl/default.php you can replace:

JHtml::_('formbehavior.chosen', 'select');

with something like:

JHtml::_('formbehavior.chosen', '.chosen');

$doc = JFactory::getDocument();

$script = "
    (function($){
        $(document).ready(function () {
            $('select:not(#page-permissions select, #page-filters select)').addClass('chosen').chosen();
        });
    })(jQuery);
";

$doc->addScriptDeclaration($script);

That will avoid to load chosen for the select fields in the permission and text filters tabs. BTW the issue is still there I think but I hope the concept is clear and is something that doesn't force us to change 170 files.

avatar Bakual
Bakual - comment - 20 Sep 2013

@phproberto Your suggestion is exactly what we are doing, only that we are using the class advancedSelwct which ia the default behavior of our chosen function.

avatar phproberto
phproberto - comment - 20 Sep 2013

@Bakual is not the same because this proposal edits 170 files.

In CSS nobody uses camelCased classes ( http://csswizardry.com/2010/12/css-camel-case-seriously-sucks/ ). We cannot change the default class that was decided time ago but we can avoid to add it to all our markup.

avatar Bakual
Bakual - comment - 20 Sep 2013

Ah sorry, I see what you did. But I don't think that's a better solution. Looks more like a workaround and is not much better than using chosen with the select selector :)
The idea is not to disable it for the permissions tab, it is to be able to disable it globally. Or for specific fields. And also that a component doesn't affect a module by applying chosen to its select. Ask @betweebrain about this :)

avatar phproberto
phproberto - comment - 20 Sep 2013

The select fields are standard fields returned from the core and they are converted to "advanced selector" at the template level. So adding that by default is wrong.

The correct way to add classes to the form XML is to describe the fields at the CMS level. Something like jform-field globally, jform-select.... And then maybe add automatic classes to specify the class of this concrete field like field-TYPE.

How do you expect to disable chosen "for specific fields"?

And also that a component doesn't affect a module by applying chosen to its select. Ask @betweebrain about this :)

You are causing the same problem. If @betweenbrain wants the module to use a field that doesn't load chosen he will need to first know that advancedSelect is what is creating the issue (this is where having a .chosen class would be better) and then add a dummy class like class="cucu" to avoid the class to be loaded. If not the template will keep loading chosen and the field used will keep adding a class that nobody expects to be there.

avatar Bakual
Bakual - comment - 20 Sep 2013

It's not perfect, but better than it is today. And I don't see a better way.

avatar betweenbrain
betweenbrain - comment - 20 Sep 2013

I do agree that a class name like advancedSelect isn't very semantic or meaningful. chosen makes more sense to me. But, I'm not about to take hard stand on that :wink: And yes, camelCase classNames seriouslySuck.

But, in my opinion, having to add a dummy class makes even less sense. If I don't add a class, no classes should be applied. If changing this behavior is a B/C issue, now is the time to fix it before LTS.

avatar Fedik
Fedik - comment - 20 Sep 2013

thoughts about the class naming is right but it part of the other topic,
I think not so difficult to search and replace advancedSelect to something like field-chosen or field-select2 or any other in the future...
if will be a clear decision then we can change it here of course

How do you expect to disable chosen "for specific fields"?

very simple, just remove the class advancedSelect for "specific fields" ;)

this pull is a simple way for allow to disable the chosen, in case if user have a problem .... but of course only for the joomla! core extensions

 ...$('select:not(#page-permissions select, #page-filters select)') ...

I think it just another tricky solution ;)

this proposal edits 170 files

yeah, but it already edited, so should be no big problem ;)
and here no big changes, I just added the class for select fields, and "switcher" in to the Isis options....

avatar Bakual
Bakual - comment - 20 Sep 2013

@betweenbrain I may be wrong but I thought B/C breaks are not allowed in STS. Earliest and only chance will be 4.0.

For frontend, chances are high that a field has a (bootstrap) class applied already. At least when I looked at form definitions a lot fields had a class like inputbox or similar.

avatar betweenbrain
betweenbrain - comment - 20 Sep 2013

@Bakual B/C may not be allowed, per se, but they do happen when necessary to fix issues (e.g. Tags API) My thinking is that now is the time to fix this before it gets into a LTS release and adoption increases. Of course, this is just my opinion.

avatar Bakual
Bakual - comment - 20 Sep 2013

Imho the name of a default selector doesn't really justify a B/C break. It's really a small issue and most will not care about it at all.

avatar betweenbrain
betweenbrain - comment - 5 Oct 2013

Just following up on this PR as chosen is still applied to all select elements by default. Is there anything that I can do to help resolve this issue?

avatar Bakual
Bakual - comment - 5 Oct 2013

I saw today that they improved the performance of the chosen plugin about 3 months ago with v0.12.0. And v.0.14.0 added a way to destroy existing chosen fields.
So maybe instead of doing a huge PR it would be sufficient to update chosen to v0.14.0.
v1.0.0 is probably not possible because it contains B/C breaks due to CSS prefix and JS event renaming.
See https://github.com/harvesthq/chosen/releases for more informations.

I didn't have the time yet to do some tests with v0.14.0 but from reading it may solve our issues.

avatar Bakual
Bakual - comment - 5 Oct 2013

Did some short tests and there is one issue which is in fact a known issue since v0.9.15. When the field is hidden during initializing, it will have a width of 0px. It could be fixed by setting a width in the chosen loading call, but for us that doesn't work due to different sizes of the selects.
There are several PRs open for this on their repo, one of them is harvesthq/chosen#1439. Unfortunately my JavaScript skills are too low to figure out how to apply this patch and test it. It apparently needs some coffee to do that :-) Someone around who knows this stuff?
If we want to upgrade our chosen plugin, we would for sure have to apply this patch to our version first.

avatar Fedik
Fedik - comment - 6 Oct 2013

upgrade it is good idea, I seen new version moth ago, but thought that upgrade not really possible cause we have already "hacked" version... no?

ok, if do upgrade, why not for last version 1.0?
search/replace for liszt: => chosen: and chzn- => chosen- should be not big a problem, and I not think that it make a big problem for extension developer, just need a warn them (in release note or somehow) ... other way we cannot do it until j4 ... but chosen developers can make a lot more useful fixes until this time (as example test, improve and apply suggested patch 1439) , and we will have a lot more extensions that will use "old" chosen, so upgrade then will be bigger problem, no?
déjà vu :)

avatar Bakual
Bakual - comment - 6 Oct 2013

There are indeed quite a few changes made to chosen, I didn't know this. Mainly for RTL support and Joomla language strings. The RTL stuff maybe is no longer needed due to fixes made on their side, but that needs to be tested first.

Reapplying those changes and a fix for the width issue will have to be done first then. Unfortunately I lack the JavaScript (and CoffeeScript?) skills to do that myself.

As for 1.0: I don't see that we should break existing templates and extensions interacting with chosen. Changes are good that we will not update chosen for the rest of J3.x anyway.

avatar phproberto
phproberto - comment - 6 Oct 2013

The fix for width 0 for hidden containers is already fixed in the core
implementation.

About the 1.0 we cannot search listz and replace it with chosen but we can
do the reverse thing. That would allow us to use the latest version keeping
B/C.

We will need to add the translation strings and the other tweaks applied
but I think they are easy to track with a diff viewer.

About the function calling chosen we should also add an extra array
parameter for the $options that chosen already support but the JHtml field
is not receiving. That can be done without breaking B/C.

In my opinion performance is not going to be better with the latest
version. I already did some tests with select2 and it was the same.

But being uptated is always good as we already did with other libraries.
El 06/10/2013 15:03, "Thomas Hunziker" notifications@github.com escribió:

There are indeed quite a few changes made to chosen, I didn't know this.
Mainly for RTL support and Joomla language strings. The RTL stuff maybe is
no longer needed due to fixes made on their side, but that needs to be
tested first.

Reapplying those changes and a fix for the width issue will have to be
done first then. Unfortunately I lack the JavaScript (and CoffeeScript?)
skills to do that myself.

As for 1.0: I don't see that we should break existing templates and
extensions interacting with chosen. Changes are good that we will not
update chosen for the rest of J3.x anyway.


Reply to this email directly or view it on GitHub#2018 (comment)
.

avatar Bakual
Bakual - comment - 6 Oct 2013

I think the difference between v1.0 and v0.14 is only the renaming. There are no other changes between them as far as I see.

Allowing extra parameters for the chosen call would be awesome if that's possible.

avatar Fedik
Fedik - comment - 6 Oct 2013

I think the difference between v1.0 and v0.14 is only the renaming ...

for current time, yes ... but for sure will be more in the future ;)

About the function calling chosen we should also add an extra array parameter for the $options ...

you read my brain :D
I already tried, but not made the pull

we cannot search listz and replace it with chosen but we can do the reverse thing...

why not?
and what you mean with the "reverse thing"? sorry, not very understand

avatar betweenbrain
betweenbrain - comment - 8 Oct 2013

Upgrading Chosen aside, where does this PR stand in terms of fixing the flawed implementation? I am testing this PR and do see Chosen not applied to any select elements.

avatar Fedik
Fedik - comment - 8 Oct 2013

after this pull the Chosen will applied only for <select class="advancedSelect"> and if Chosen enabled in template configuration (enabled by default ) ...
or it not works for you?

Upgrading Chosen do no fix a performance issue, but there some other fixes inside

avatar betweenbrain
betweenbrain - comment - 15 Oct 2013

@Fedik when testing via patchtester, I get

The file marked for modification does not exist: administrator/components/com_config/models/forms/application.xml

However, when testing via $ git clone -b chosen-disable-enable-3 https://github.com/Fedik/joomla-cms.git, I have a good test!

It will be nice to get this fixed. Nice work!

avatar betweenbrain
betweenbrain - comment - 15 Oct 2013

PS: What is the appropriate tracker item for this?

avatar mbabker
mbabker - comment - 15 Oct 2013

Needs to be synced up to master. One of the GSoC projects moved some files around which is causing the issue here.

avatar Fedik
Fedik - comment - 15 Oct 2013

synced & updated

avatar betweenbrain
betweenbrain - comment - 16 Oct 2013

Just rested against master and it applied fine and works great. Thanks!

avatar betweenbrain
betweenbrain - comment - 22 Oct 2013

Any update on this PR?

avatar Bakual
Bakual - comment - 22 Oct 2013

@betweenbrain I think without tests logged in JoomlaCode (Tracker: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29641 as in first comment here), this will never get merged at all. The tracker is still only "confirmed", alltough a patch was offered and it should be "pending".

avatar betweenbrain
betweenbrain - comment - 22 Oct 2013

@Bakual Thanks. I had asked if that was the right tracker at #2018 (comment), but didn't get any response. I will add my test result there now.

avatar Achal-Aggarwal
Achal-Aggarwal - comment - 22 Oct 2013

Take a look on Achal-Aggarwal@joomla:master...Achal-Aggarwal:rem-chosen for a more simplified solution.

avatar betweenbrain
betweenbrain - comment - 22 Oct 2013

@Achal-Aggarwal is there a PR for that?

avatar Achal-Aggarwal
Achal-Aggarwal - comment - 23 Oct 2013

Not now, But if you want to test it use https://github.com/Achal-Aggarwal/joomla-cms/compare/joomla:master...Achal-Aggarwal:rem-chosen.diff I was trying to throw an idea in the pool to consider.

Here is PR #2312 but don't consider it final, I am also thinking of adding a text field below the list field in the template params to get the class on which selected jQuery plugin to apply or not to apply.

avatar Bakual
Bakual - comment - 24 Oct 2013

I think updating chosen to the latest compatible build (v0.14.0 before namespace change) could solve most issues. See #2228

It should be faster according to their release notes and allows to "Un-Chosen" selects.

avatar Fedik
Fedik - comment - 24 Oct 2013

I did not get a visible speed up on chosen 0.14 ...
maybe I just not know what they made faster there :D

avatar Bakual
Bakual - comment - 24 Oct 2013

I was reading https://github.com/harvesthq/chosen/releases/tag/v0.12.0
But now when rereading it only addresses the issue when there is a huge amount of options. This is what I originally tried to address in my PR (#1620) by automatically disabling chosen for large selects.

The performance issue you see is probably caused by the big amount of chosen selects itself (like in the permission tab or configuration in general). This is indeed a different issue and unfortunately not solved by upgrading chosen. You're right here.

avatar betweenbrain
betweenbrain - comment - 24 Oct 2013

Part of the issue that this PR solves is that Chosen is currently applied to all <select> element, which breaks functionality for some extensions, and overall bad practice.

avatar Bakual
Bakual - comment - 24 Oct 2013

@betweenbrain Yeah, that's an important fix as well. Actually the one I care most about :)

avatar betweenbrain
betweenbrain - comment - 24 Oct 2013

@Fedik I think @davidhurley mentioned on Twitter that this PR is not mergeable.

avatar betweenbrain
betweenbrain - comment - 24 Oct 2013

@betweenbrain Yeah, that's an important fix as well. Actually the one I care most about :)

:thumbsup:

avatar Fedik
Fedik - comment - 24 Oct 2013

@betweenbrain ok, I will try check and sync if need ... when will get some time ;)

avatar Fedik
Fedik - comment - 26 Oct 2013

meanwhile #2228 Update chosen.js to v0.14 ready for testing

avatar Fedik
Fedik - comment - 26 Oct 2013

synced & updated again

avatar marceloleart
marceloleart - comment - 27 Nov 2013

Today i've had to change /administrator/components/com_config/view/component/tmpl/default.php # 18

JHtml::('formbehavior.chosen', 'select');
to
JHtml::
('formbehavior.chosen');

because of the mentioned issues with large amounts of select items in the permissions tab in com_config view=component.

I think that it is definitely not necessary to invoke chosen in this view because there are only small selects with 3 options each. Could someone take this and try another update please?

avatar Bakual
Bakual - comment - 27 Nov 2013

@marceloleart I think if we get enough testers for upgrading chosen (#2228) we would have a few more options for how to deal with stuff like this.

avatar betweenbrain
betweenbrain - comment - 27 Nov 2013

I think if we get enough testers for upgrading chosen (#2228) we would have a few more options for how to deal with stuff like this.

Agreed. But I do feel that this patch needs merging as Chosen is still blindly applied to all select elements.

avatar marceloleart
marceloleart - comment - 27 Nov 2013

I will try to get some time for contributing but updating chosen will not solve the mentioned performance issues in those views where every select element is getting chznd. Maybe this is the wrong ticket but i thought its a good idea writing my +1 here because @Fedik mentioned something like that too:

Fedik commented 2 months ago
performance is the main reason for skip it for "Permission Settings", I noticed that the "Global configuration" page (for example) works a lot faster if disable Chosen just for "Permission Settings" ;)"

avatar Bakual
Bakual - comment - 27 Nov 2013

@marceloleart @betweenbrain Agreed that we need a change in how we apply chosen to all selects. That's just not a good way currently. I'm not sure if an option in the template is the answer, but using the advancedSelect class as the trigger is certainly the right way and how it was intended from beginning. Devs just got lazy and applied it to all select instead.
It mainly needs more testers to even be considered for merge.

but updating chosen will not solve the mentioned performance issues in those views

True, but it solves some other related issues where you can't disable chosen for a single select. With the updated chosen that would be possible.

avatar betweenbrain
betweenbrain - comment - 27 Nov 2013

I'm not sure if an option in the template is the answer

@Bakual would you be in favor of this PR if @Fedik reverted Fedik@cf3a83f ?

avatar Fedik
Fedik - comment - 28 Nov 2013

the option in the template gives me simple way for disable this thing, and do not bother me again if I do new back-end template ... also I can add something else (select2 example) without problem.
basically the chosen.js it just a styling trick, so no sense put it in the each tmpl file..

avatar Bakual
Bakual - comment - 28 Nov 2013

@betweenbrain @Fedik Thinking about it a bit more, I think it's indeed a good idea to put this call into the template. Design should be handled by the template, not the layoutfile.

The only downside is that chosen is loaded on all pages, regardles if it's needed or not. But for backend this will not really be an issue as a select is present on almost all pages.

If we want to add this to frontend (which isn't in the scope of this PR), it would probably need a better solution. Most likely using some JLayout.

avatar Fedik
Fedik - comment - 28 Nov 2013

about JLayout, @phproberto already suggested #2193

avatar Bakual
Bakual - comment - 28 Nov 2013

Aye, it's similar. I think JLayout can be a solution for almost all JHtml classes. If we move markup and JS loading to a layout, templates can override the layout and load select2 instead of chosen or whatever they want to have changed. The possibilities are endless :-)
But it's offtopic a bit.

avatar betweenbrain
betweenbrain - comment - 9 Dec 2013

Unless anyone is opposed, I'd like to get this in for the 3.2.1 release. @Fedik do you mind re-syncing your PR, hopefully one last time? Thanks!

avatar Fedik
Fedik - comment - 9 Dec 2013

yes, I will try find a some time for this in next 1-3 days

avatar Fedik
Fedik - comment - 9 Dec 2013

ok, synced & updated again

avatar betweenbrain
betweenbrain - comment - 9 Dec 2013

Thanks @Fedik! This tests good for me again, and the code looks good as well. My only suggestions is that one language string change. I'm ready to hit the merge button.

avatar Fedik
Fedik - comment - 11 Dec 2013

@betweenbrain description changed

avatar betweenbrain betweenbrain - reference | - 11 Dec 13
avatar betweenbrain
betweenbrain - comment - 11 Dec 2013

Thanks @Fedik! Looks like the language freeze is still in effect, so we can't merge this until that is lifted when 3.2.1 is released (hopefully very soon!).

avatar betweenbrain
betweenbrain - comment - 20 Dec 2013

@mbabker @Bakual are we able to merge this now?

avatar betweenbrain betweenbrain - close - 20 Dec 2013
avatar betweenbrain betweenbrain - change - 20 Dec 2013
Status New Closed
Closed_Date 0000-00-00 00:00:00 2013-12-20 19:52:24
avatar betweenbrain betweenbrain - close - 20 Dec 2013
avatar betweenbrain
betweenbrain - comment - 20 Dec 2013

Thanks @Fedik and everyone who helped test and review this change!

avatar Bakual
Bakual - comment - 20 Dec 2013

Wohoooo! :+1:

avatar infograf768 infograf768 - reference | - 21 Dec 13
avatar infograf768 infograf768 - reopen - 21 Dec 2013
avatar infograf768
infograf768 - comment - 21 Dec 2013

I reverted this as it breaks Joomla installation.

avatar infograf768 infograf768 - change - 21 Dec 2013
Status Closed New
avatar infograf768 infograf768 - reopen - 21 Dec 2013
avatar Fedik
Fedik - comment - 21 Dec 2013

@infograf768 have a more details? cause here no any changes for /installation

avatar infograf768
infograf768 - comment - 21 Dec 2013

@Fedik

[... other errors...]
[21-Dec-2013 09:27:58 UTC] PHP Warning: simplexml_load_file() [<a href='function.simplexml-load-file'>function.simplexml-load-file</a>]: /Applications/MAMP/htdocs/stagingcms/modules/mod_articles_news/mod_articles_news.xml:76: parser error : Attribute class redefined in /Applications/MAMP/htdocs/stagingcms/libraries/cms/installer/installer.php on line 1929
[21-Dec-2013 09:27:58 UTC] PHP Warning: simplexml_load_file() [<a href='function.simplexml-load-file'>function.simplexml-load-file</a>]: class=&quot;advancedSelect&quot;&gt; in /Applications/MAMP/htdocs/stagingcms/libraries/cms/installer/installer.php on line 1929
[21-Dec-2013 09:27:58 UTC] PHP Warning: simplexml_load_file() [<a href='function.simplexml-load-file'>function.simplexml-load-file</a>]: ^ in /Applications/MAMP/htdocs/stagingcms/libraries/cms/installer/installer.php on line 1929
[21-Dec-2013 09:27:58 UTC] PHP Warning: simplexml_load_file() [<a href='function.simplexml-load-file'>function.simplexml-load-file</a>]: /Applications/MAMP/htdocs/stagingcms/modules/mod_articles_news/mod_articles_news.xml:76: parser error : Attribute class redefined in /Applications/MAMP/htdocs/stagingcms/libraries/cms/installer/installer.php on line 2141
[21-Dec-2013 09:27:58 UTC] PHP Warning: simplexml_load_file() [<a href='function.simplexml-load-file'>function.simplexml-load-file</a>]: class=&quot;advancedSelect&quot;&gt; in /Applications/MAMP/htdocs/stagingcms/libraries/cms/installer/installer.php on line 2141
[21-Dec-2013 09:27:58 UTC] PHP Warning: simplexml_load_file() [<a href='function.simplexml-load-file'>function.simplexml-load-file</a>]: ^ in /Applications/MAMP/htdocs/stagingcms/libraries/cms/installer/installer.php on line 2141

avatar Bakual
Bakual - comment - 21 Dec 2013

Sounds like an error in an XML file to me.

avatar Fedik
Fedik - comment - 21 Dec 2013

yes, looks like the class duplication in mod_articles_news.xml .. I will check more

avatar infograf768
infograf768 - comment - 21 Dec 2013

we have the error in 2 modules:
breadcrumbs and article_news where we have twice a class declaration.
But anyway, after a discussion in the maintainers chat, it is judged that this can't be merged in a 3.2.x release for B/C reasons

avatar Fedik
Fedik - comment - 21 Dec 2013

ok, duplication fixed ... think it happened during one of the sync

@infograf768 whether was offered some alternative?

avatar Bakual
Bakual - comment - 21 Dec 2013

@fedik A solution that was thrown into the room was using JLayouts instead to render the select boxes. And then use overrides for different uses.
That probably would work also for 3rd parties without issues. Since the last iteration of JLayouts, I think could even selectively override a layout for a single component or view. But I may be wrong here. @phproberto would know better and I think he wrote a good post somewhere how this works.

avatar wilsonge
wilsonge - comment - 21 Dec 2013

http://docs.joomla.org/Layout_Overrides_in_Joomla Roberto's JLayout article is also on the JDocs of course :)

avatar betweenbrain
betweenbrain - comment - 21 Dec 2013

@Bakual can you explain how I could, for example, create a module and ship it so Chosen.js isn't applied to it?

avatar betweenbrain
betweenbrain - comment - 21 Dec 2013

But anyway, after a discussion in the maintainers chat, it is judged that this can't be merged in a 3.2.x release for B/C reasons

Can we then merge it into a 3.3.0 branch then? There is supposed to be one created for exactly these types of situations.

avatar Bakual
Bakual - comment - 21 Dec 2013

@betweenbrain I would have to look into JLayout again, but I think I read you can specify layouts "overrides" for specific extensions. Also I think Roberto has a PR open which would allow to specify an alternate layout for a field in the XML. This one would probably solve your issue with the module as well.

avatar betweenbrain
betweenbrain - comment - 21 Dec 2013

@Bakual to be honest, I'm not sure if I agree with the idea of using JLayouts and overrides to handle this, but I am open to exploring it as I may not fully understand the implementation for solving the issue.

I do feel that the current approach of using a class like advancedSelect is a very valid way to solve this. Chosen.js should be opt-in, not opt-out.

avatar Bakual
Bakual - comment - 21 Dec 2013

I do feel that the current approach of using a class like advancedSelect is a very valid way to solve this. Chosen.js should be opt-in, not opt-out.

@betweenbrain I agree that we MUST change the calls from JHtml::('bevahior.chosen', 'select') to JHtml::('bevahior.chosen'). It's currently just wrong and disables every attemp to customise anything, even when using JLayouts.
The question imho is only how to add the class. It could be done in the JLayout which would allow it to be overriden by templates and extensions.
So JLayout is not the solution itself, but it may help.

avatar betweenbrain
betweenbrain - comment - 21 Dec 2013

Would it be possible to JLayout, within the template, to enable or disable
chosen as well as to set the class the extension needs to use for chosen to
target it? That way, template devs can change the call for chosen,
including globally targeting all select elements or changing the class, and
extensions can opt-in by just adding a class to their select elements?

Best,

Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain

Sent from mobile. Please pardon any typos or brevity.
On Dec 21, 2013 9:56 AM, "Thomas Hunziker" notifications@github.com wrote:

I do feel that the current approach of using a class like advancedSelect
is a very valid way to solve this. Chosen.js should be opt-in, not opt-out.

@betweenbrain https://github.com/betweenbrain I agree that we MUST
change the calls from JHtml::('bevahior.chosen', 'select') to
JHtml::('bevahior.chosen'). It's currently just wrong and disables every
attemp to customise anything, even when using JLayouts.
The question imho is only how to add the class. It could be done in the
JLayout which would allow it to be overriden by templates and extensions.
So JLayout is not the solution itself, but it may help.


Reply to this email directly or view it on GitHub#2018 (comment)
.

avatar Bakual
Bakual - comment - 21 Dec 2013

I don't know what exactly is possible with JLayouts. That is something Roberto would need to answer.

avatar mbabker
mbabker - comment - 21 Dec 2013

Ask and ye shall receive. 3.3-dev branch open for business.

On Sat, Dec 21, 2013 at 7:07 AM, Matt Thomas notifications@github.comwrote:

But anyway, after a discussion in the maintainers chat, it is judged that
this can't be merged in a 3.2.x release for B/C reasons

Can we then merge it into a 3.3.0 branch then? There is supposed to be one
created for exactly these types of situations.


Reply to this email directly or view it on GitHub#2018 (comment)
.

avatar Fedik
Fedik - comment - 21 Dec 2013

as I understand, with JLayouts it should looks like:

  • remove JHtml::('bevahior.chosen', 'select'), and put JHtml::('bevahior.chosen') in the template
  • make the layout for each <select> field input type: list, category and so on
  • overwrite these layouts in the template, add there class advancedSelect
avatar Bakual
Bakual - comment - 21 Dec 2013
  • remove JHtml::('bevahior.chosen', 'select'), and put JHtml::('bevahior.chosen') in the template
  • make layout for each field input type: list, category and so on
  • overwrite these layouts in the template, add there class advancedSelect

If we make changes to the fields (like adding a JLayout), it will always be for frontend and backend. I think the advancedSelect class would have to be added on the default layout. And the template can override it if it wants. It could then add its own class and call JHtml with this custom class. One could even add a parameter to enable/disable this custom call.
The JHtml call would have to be back at the components level, otherwise it will fail in frontend and for other backend templates, and thus not be full backward compatible.

Just an idea. It sure needs more thinking.

avatar infograf768
infograf768 - comment - 22 Dec 2013

But anyway, after a discussion in the maintainers chat, it is judged that this can't be merged in a 3.2.x release for B/C reasons

Can we then merge it into a 3.3.0 branch then? There is supposed to be one created for exactly these types of situations.

Once there is an agreement on the way to proceed and we are sure we are B/C, sure.

avatar betweenbrain
betweenbrain - comment - 22 Dec 2013

@infograf768 What is the B/C issue? I don't think that has ever been stated.

avatar betweenbrain
betweenbrain - comment - 2 Jan 2014

@infograf768 can you tell us what the B/C is?

avatar phproberto
phproberto - comment - 3 Jun 2014

I'm going to throw this on the frontenders working group to unblock it with their guidance

avatar brianteeman
brianteeman - comment - 27 Jul 2014

Closed as per the comment in the tracker

avatar brianteeman brianteeman - change - 27 Jul 2014
Status New Closed
Closed_Date 2013-12-20 19:52:24 2014-07-27 11:08:08
avatar brianteeman brianteeman - close - 27 Jul 2014
avatar brianteeman brianteeman - close - 27 Jul 2014
avatar betweenbrain
betweenbrain - comment - 27 Jul 2014

@phproberto if you do tackle this with the frontenders group, please also see #3721

Add a Comment

Login with GitHub to post a comment