This thing that Jissues constantly changes the description of the PR is totally annoying. Anyhow, there is one more thing with this PR:
##### The slider effect does NOT work therefore:
1. the first box, where someone was supposed to select the filter branch, is now hidden
2. the rendered output is a bunch of boxes
3. if we are about to fix this we need to redesign the page (or rewrite the component) and use jQuery!

@test. Failed. First box is missing or hidden. Other boxes (which are supposed to be hidden initially, then slide out when selected from the first box) are shown.

@chrisdavenport The change of behavior was intended, please read my comment above.

@dgt41 Sorry, I thought you were describing how it was failing, not how you were changing its behaviour! My bad.

Okay, functionally it seems to work, but there do seem to be display issues. When I first go in to create a new filter, all the boxes are displayed vertically without borders, but when I save a new filter then go back in to edit it, the boxes are displayed in rows of 2 and the last box has a right border and a horizontal scroll-bar. Can you make it look a bit prettier? Maybe just use more of the available screen space and restore the box borders?

Tested in Firefox and Chrome on Ubuntu 14.04.

@chrisdavenport No prob. I think it is waste of time to try to correct this outdated code, thus I propose to move it altogether to jquery. Anyway I comment out some more lines in the script and placed a border around every filter box. Here is a quick render:

@test Fixes the issue. Thanks.

A PR for moving to jQuery would be welcome. However, the priority right now is to get the current code to work properly since this bug is a release blocker for 3.4.

@chrisdavenport I know this is a dirty quick patch, but it might be helpful!

I know this is more of house keeping than anything else - but if we comment lines of code out - shouldn't these lines not be included in the PR? It will be there historically in the GIT repo. Just a suggestion.

@chrisdavenport @spignataro OK I think this one is a better solution, as it restores the previous functionality!
Personally speaking I don’t like the old behavior (the slide effect), I prefer the many boxes edition...

The "Select All" checkbox doesn't work.

Hmm. I agree that the slide effect is not really good and we should just drop it. It causes problems and you've proved that it isn't really required. I also suspect that it would not work well for RTL languages.

It's also less JavaScript to maintain (and convert to jQuery) if we just get rid of it.

@chrisdavenport It’s your decision what will make it to Joomla. Picking the first two commits, you’ll get the plain boxes, applying all 3 you'll get the slide effect as well...

Not just my decision. Personally, I'd go with just the plain boxes.

@dgt41 Since no-one else has commented to the contrary, I'm going to make the decision and say we should go with the plain boxes and forget the slide effect. Can you adjust the PR to reflect that?

@chrisdavenport Chris I did the changes here, also as you can see I also changed the way that slider finder.js is injected in order to make sure that behavior.framework is also called components/com_finder/helpers/html/filter.php. This was broken on the last changes moving the back end to jquery, so this PR right now patches two bugs ????

@dgt41 There is a new problem with this PR:

1. Click New to create a new filter.
2. Tick a few checkboxes but do NOT enter a title.
3. Click Save and Close. You should get a warning about the missing title.
4. Fill in the title field and click Save and Close again.
5. Filter is saved but is listed as having a map count of 0 and indeed no filter settings were saved.

@chrisdavenport Confirmed. The problem for this instance is that there is no client side validation on the title, so data is sent to server -> will not validate thus not saved -> form will reload with empty (not selected) fields. Also changing line 12 @ /administrator/com_finder/views/filter/tmpl/default.php to

JHtml::_('behavior.formvalidation');

which was the prior code here gives the same result, thus this is not a new problem. Anyways I am gonna try to find a way around it...

@chrisdavenport Done! Client side validation is activated

@dgt41:
I'm getting different JS errors in console:

Without:

With:

Is this expected?

what url?

/administrator/index.php?option=com_finder&view=filter&layout=edit

It’s not an error it’s a warning and nothing we can do about it (first comes from jquery second from mootools)

you don't have the a is undefined ?

No. Are you on staging?

staging + #5364

Can you save? If title is blank you get an alert? If saved, and reopen checked fields are ok?

confirmed: I have the error bothe while creating new filter and when opening existing filter.

Also... I check "Author" in create new filter, save, re-open and... author is unchecked. This both with/without this PR...

Sorry, Dimitris: I have to go out again and probably I'll be out for dinner... I will ping you on Skype when I'll be back. Don't give the a is undefined error too much weight: can be due to local config. Will check with Chrome later...

@test Success. No JavaScript errors for me. All working as expected. Thanks @dgt41

@dgt41

For saving without title I have this:

undefined
Invalid field:  Title 

Checkboxes in first column have no effect (not saved)

I'm still getting JS error. In Chrome the error diagnostic is different but on the same MT line:

Now installing on a different server to double check...

@smanzi
For the first one there is a PR #5366
I cannot replicate the second and the third

OK... let me check on new (on-line) server. In case I'll give you access credentials to it...

@dgt41 strange things happening...
I know why you couldn't replicate my issue with saving 1st column: your 1st different from mine! Look what I had...

Now, after disabling 5364 and then enabling it again I have this, which I suppose is what you have:

Not finished installing on on-line server yet... report in some minutes...

on-line I have this, but site not multilingual and thus language switcher not enabled:

NO JS error on-line, but site still not multilingual...

Site doesn't need to be multi-lingual to test this. It really shouldn't make any difference.

@chrisdavenport Yeah, I'm sure you're right but, I'm trying to figure out what could it be... Have you seen those anomalous 4 columns?

@dgt41 #5366 doesn't solve "undefined", not even on new on-line site: want to come and see? Now only #5099 and #5366 are applied. Ping me on Skype...

@smanzi The undefined is a problem on core.js, unrelated to this should be fixed in #5366, right now it’s not. You can try an empty title in an article…

Good! Then... No error on the new site... (WTF!) And... (I know this too is unrelated, but...) those 4 columns??? Absurd: now I don't have them any more (after disabling and re-enabling #5364)...

Caching...

I think you may have had an earlier version of the patch installed. You should not see boxes labelled "Search All" or "Search by Language". In your case you should just have "Search by Author", "Search by Category" and "Search by Type".

@smanzi can you do one more test? On the first site that you had 5 columns remove the patch
purge and re index
apply the patch
You still have 5 columns?

@chrisdavenport, can be... @dgt41 browser caching, no (cleaned on every browser exit) com_patchtester caching always disable...

@dgt41 can't replicate. I tried:

no patch -> purge/index -> apply patch -> search filter
and also
patch -> purge/index -> remove patch -> search filter

always 3 columns...

New commit: should re-test?

with new commit too: @test success

@chrisdavenport if it is OK for you too, can you please give @test in Jissues too (http://issues.joomla.org/tracker/joomla-cms/5099) so this can go RTC?

It should already be there. Everything in Github is automatically replicated to JIssues.

nope... @tests must be given explicitly in issues.joomla.org, unhappily... go and see... there is only mine...

Scroll back about 20 messages and you should see mine.

I know, Chris, but try going here: http://issues.joomla.org/tracker/joomla-cms/5099 and look at the right column, under Tests:...

basically @tests given here do not count...

Ahh, okay. I didn't know that feature existed. Done.

Yes, now it is OK!! Thanks!

I learn something new every day. Thanks for teaching me. :-)

Everybody does!!

Tested successfully! Important to know: You have to clean the cache of the browser because we have changes in CSS and JS files.

Thank you for the fix, @dgt41!

3 successful tests -> RTC


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

Houston, I have a problem:
Boxes do not accomodate when more items than their height here (macintosh Firefox)
see search by category:

This is because we have and overflow:hidden in the <dl
once deleted we can manage:

changing back to pending

@dgt41 Could you please adjust the CSS instructions?

It is added as inline style and overflow:auto; from .checklist is overriden
<dl class="checklist" rel="tax-580" style="overflow: hidden;">

see slidefilter.js , line 91
changing to auto solves the issue here, except that the horizontal scroll is always on (but better anyway)

OR, we could just delete that line and change the .checklist css to use
overflow-x: hidden;
which would solve the horizontal scroll bar
forget it: IE only gets it in version 9 (grr)

@Kubik-Rubik @infograf768 I adjusted the script to have overflow x, y (hidden and auto). Can you give it a spin

Here seems ok:

@dgt41: I think there may be problems using overflow-y': 'auto', 'overflow-x': 'hidden:

• not supported by IE8 (do we care ?)
• what if elements longer than 222px ?

@smanzi I meant to write that but put it in the wrong order
@infograf768 IE8 seems happy here

@dgt41... curious... I didn't tried (I don't have IE8 at hands now...) but according to my doc I agree with @infograf768 with they being not supported on IE8...

element.set('styles', {'overflow-y': 'auto', 'overflow-x': 'hidden'});

I do not think we need the auto.
I personally would rather delete that line and change the property in the .checklist css to use only
overflow-x: hidden;
as far as I understand, it understands that in this case y is auto

@infograf768 seems cleaner that way! changes are coming...

to be safe (and if we really want to hide x overflow) I think the best is to use not JS but inline style overflow: auto; overflow-y: auto; overflow-x: hidden so that if overflow-y is not handled the plain overflow will rule...

Actually my tests were wrong (different joomla version on the tests) IE8 GOT problems:
What do you think about

        element.set('styles', {'-ms-overflow-y': 'auto', '-ms-overflow-x': 'hidden'});

why ms- ?

@infograf768 -ms to get IE8 work with overflow-x, y
This is only css changes:

.checklist {
    border: 1px solid #ccc;
    height: 220px;
    overflow: auto;
    width: 220px;
    float: left;
    margin: 0;
    overflow-x: hidden;
    -ms-overflow-y: auto;
    -ms-overflow-x: hidden;
}

@infograf768 I got it from here: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-x

@dgt41: try my solution (also not inline...): it is Cascading Style Sheets...

@smanzi we are on the same page here

@dgt41 no prob: last attribute declared takes precedence if understood...

I still can't test with IE8, but it seems that the info that overflow-x and overflow-y are not supported by IE8 is false. Anyway at http://reference.sitepoint.com/css/overflow we have:

Internet Explorer version 8 has a variety of bugs when overflow is combined with max-width or max-height and you can find a series of test cases documented at the Hilbrand Edskes site.

and at http://www.w3schools.com/jsref/prop_style_overflowx.asp

Note: The overflowX property does not work properly in IE8 and earlier.

From Microsoft:

http://msdn.microsoft.com/en-us/library/ie/ms530826%28v=vs.85%29.aspx

Remarks

Windows Internet Explorer 8. The -ms-overflow-x attribute is an extension to CSS, and can be used as a synonym for overflow-x in IE8 Standards mode. "

@infograf768 my reading is that you can use both: either -ms-overflow-x or overflow-x... correct?

Mine is that we should be careful and do as was proposed above. AND test in IE8.

.checklist {
    border: 1px solid #ccc;
    height: 220px;
    overflow: auto;
    width: 220px;
    float: left;
    margin: 0;
    overflow-x: hidden;
    -ms-overflow-y: auto;
    -ms-overflow-x: hidden;
}

test... for sure! Damn, do I have to install a Windows XP VM??

@infograf768 This works fine here

.checklist {
    border: 1px solid #ccc;
    height: 220px;
    overflow: auto;
    width: 220px;
    float: left;
    margin: 0;
    overflow-x: hidden;
    -ms-overflow-y: auto;
    -ms-overflow-x: hidden;
}

Could someone look at making finder rtl aware?
We could have a single rtl.css file for it that would be loaded when the ltr default css are, and after these, in order to correct some stuff>

For example here in the case of .checklist, we would need a float: right;

Should be possible. That was one reason I favoured dropping the sliders. Needs a front-ender to look into it. I suggest opening a separate issue.

@infograf768 @chrisdavenport is this acceptable:

if (JFactory::getDocument()->direction == 'rtl')
{
    JFactory::getDocument()->addStyleDeclaration('
.checklist, .checklist dd, #branch-selectors dd, .checklist div.control-group, #branch-selectors div.control-group { text-align: right; }
dl.checklist.dt.label.checkbox { text-align: center; }
.radio input[type="radio"], .checkbox input[type="checkbox"] { float: left; margin-right: 5px; left: 5px;}
');
}

preview:

Personnaly, I prefer full stylesheet as it is easy to correct/add
something like (concerning filters only):

diff --git a/components/com_finder/helpers/html/filter.php b/components/com_finder/helpers/html/filter.php
index 6d6c70c..6049bee 100644
--- a/components/com_finder/helpers/html/filter.php
+++ b/components/com_finder/helpers/html/filter.php
@@ -115,5 +115,10 @@
        {
            JHtml::_('stylesheet', 'com_finder/sliderfilter.css', false, true, false);
-           JHtml::_('script', 'com_finder/sliderfilter.js', false, true);
+           
+           if (JFactory::getDocument()->direction == 'rtl')
+           {
+               JHtml::_('stylesheet', 'com_finder/finder-rtl.css', false, true, false);
+           }
+           JHtml::_('script', 'com_finder/sliderfilter.js', true, true);
        }

@@ -433,4 +438,9 @@
        {
            JHtml::stylesheet('com_finder/sliderfilter.css', false, true, false);
+           
+           if (JFactory::getDocument()->direction == 'rtl')
+           {
+               JHtml::_('stylesheet', 'com_finder/finder-rtl.css', false, true, false);
+           }
        }

diff --git a/media/com_finder/css/finder-rtl.css b/media/com_finder/css/finder-rtl.css
new file mode 100644
index 0000000..c82097d
--- /dev/null
+++ b/media/com_finder/css/finder-rtl.css
@@ -0,0 +1,27 @@
+.checklist {
+   float: right;
+}
+
+.checklist {
+   border-right-width: 0;
+}
+
+#filter_lists div {
+   float: right;
+}
+
+.checklist dt,
+#branch-selectors dt {
+   padding: 0 2px 2px 0;
+   text-align: right;
+}
+
+.checklist,
+.checklist dd,
+#branch-selectors dd,
+.checklist div.control-group,
+#branch-selectors div.control-group {
+   padding: 0 2px 0 0;
+   text-align: right;
+}
+

Which gives:

@infograf768 Just added rtl support per your request

There are 2 places in that file where the rtl.css has to be added (see above)
Also, any reason to move the place where we load the js file ?

My thought was that this js was only used in here, maybe I am wrong...

For me, its fine now.
(other rtl stuff, concerning frontend for example, can be added later.

One more tester.

@test works for me!

Fixed with 6fb8834