Fix Finder javascript error #5099

dgrammatiko · 2014-11-13T23:22:18Z

Fixes an error on Admin side for Finder #5018

Bug report here

Test instructions

Ensure you have content (ideally sample data)

Components -> Smart Search -> Search filters.
Click on New to create a new filter.

Check your browsers console for js error

Apply this patch

Re check your browser
Check functionality

Side note

Finder is totally dependent on mootools and this has to change somehow...

dgrammatiko · 2014-11-14T15:49:14Z

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:

the first box, where someone was supposed to select the filter branch, is now hidden
the rendered output is a bunch of boxes
if we are about to fix this we need to redesign the page (or rewrite the component) and use jQuery!~~

chrisdavenport · 2014-11-14T16:10:32Z

@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.

dgrammatiko · 2014-11-14T18:08:37Z

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

chrisdavenport · 2014-11-14T23:11:05Z

@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.

dgrammatiko · 2014-11-15T00:09:46Z

@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:

chrisdavenport · 2014-11-15T00:18:24Z

@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.

dgrammatiko · 2014-11-15T00:19:54Z

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

spignataro · 2014-11-15T00:50:08Z

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.

dgrammatiko · 2014-11-15T01:53:41Z

@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...

chrisdavenport · 2014-11-15T10:52:36Z

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.

dgrammatiko · 2014-11-15T15:02:16Z

@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...

chrisdavenport · 2014-11-15T16:51:52Z

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

chrisdavenport · 2014-12-07T22:26:53Z

@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?

dgrammatiko · 2014-12-07T23:32:53Z

@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 😄

chrisdavenport · 2014-12-08T10:50:44Z

@DGT41 There is a new problem with this PR:

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

dgrammatiko · 2014-12-08T11:15:52Z

@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...

dgrammatiko · 2014-12-08T11:28:29Z

@chrisdavenport Done! Client side validation is activated

smanzi · 2014-12-10T17:14:58Z

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

dgrammatiko · 2014-12-10T17:19:03Z

@smanzi we are on the same page here

smanzi · 2014-12-10T17:20:25Z

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

smanzi · 2014-12-10T17:31:54Z

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.

smanzi · 2014-12-10T17:34:34Z

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

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

infograf768 · 2014-12-10T17:47:29Z

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. "

smanzi · 2014-12-10T17:53:56Z

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

infograf768 · 2014-12-10T17:55:49Z

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;
}

smanzi · 2014-12-10T17:58:32Z

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

dgrammatiko · 2014-12-10T18:02:56Z

@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;
}

infograf768 · 2014-12-10T18:20:02Z

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;

chrisdavenport · 2014-12-10T18:25:13Z

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.

dgrammatiko · 2014-12-10T18:39:55Z

@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:

infograf768 · 2014-12-11T09:17:20Z

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;
+}
+

infograf768 · 2014-12-11T09:20:57Z

Which gives:

…js_Finder_error

dgrammatiko · 2014-12-11T10:12:39Z

@infograf768 Just added rtl support per your request

infograf768 · 2014-12-11T10:16:54Z

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 ?

dgrammatiko · 2014-12-11T10:20:47Z

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

infograf768 · 2014-12-11T10:35:42Z

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

One more tester.

waader · 2014-12-11T13:00:32Z

@test works for me!

wilsonge · 2014-12-11T15:00:28Z

Fixed with 6fb8834

jissues-bot added the PR-staging label Nov 13, 2014

This was referenced Nov 13, 2014

Smart Search: Edit search filter sliders and checkboxes not working #5018

Closed

Smart Search / Finder Filter in the admin - unable to select all items under a specific filter #4989

Closed

dgrammatiko mentioned this pull request Nov 19, 2014

Introduce jquery formvalidator for com_finder #5045

Closed

dgrammatiko mentioned this pull request Dec 3, 2014

remove jq ready() when not needed #5255

Closed

dgrammatiko added 4 commits December 8, 2014 01:03

js error fix

88c714a

Enhancements

e19a05d

Fully restored functionality

927ce4b

Changes for boxes

1cb7ca8

dgrammatiko mentioned this pull request Dec 8, 2014

Smart Search - Corrects the ordering of loaded JS files ... #5336

Merged

Client side validation for title etc...

daf34b4

This was referenced Dec 9, 2014

Undefined titles on alert messages #5366

Merged

Fixing broken language indexing in com_finder #5364

Merged

dgrammatiko added 2 commits December 11, 2014 12:00

Merge branch 'staging' of https://github.com/joomla/joomla-cms into _…

f38b5f5

…js_Finder_error

Add rtl support

ceb432b

oops

f7f5b4a

infograf768 added the RTC This Pull Request is Ready To Commit label Dec 11, 2014

infograf768 added this to the Joomla! 3.4.0 milestone Dec 11, 2014

wilsonge closed this Dec 11, 2014

wilsonge referenced this pull request Dec 11, 2014

js error fix

6fb8834

dgrammatiko deleted the _js_Finder_error branch December 11, 2014 15:20

zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015

Uh oh!

Fix Finder javascript error #5099

Fix Finder javascript error #5099

Uh oh!

Conversation

dgrammatiko commented Nov 13, 2014

Fixes an error on Admin side for Finder #5018

Test instructions

Side note

Finder is totally dependent on mootools and this has to change somehow...

Uh oh!

dgrammatiko commented Nov 14, 2014

Uh oh!

chrisdavenport commented Nov 14, 2014

Uh oh!

dgrammatiko commented Nov 14, 2014

Uh oh!

chrisdavenport commented Nov 14, 2014

Uh oh!

dgrammatiko commented Nov 15, 2014

Uh oh!

chrisdavenport commented Nov 15, 2014

Uh oh!

dgrammatiko commented Nov 15, 2014

Uh oh!

spignataro commented Nov 15, 2014

Uh oh!

dgrammatiko commented Nov 15, 2014

Uh oh!

chrisdavenport commented Nov 15, 2014

Uh oh!

dgrammatiko commented Nov 15, 2014

Uh oh!

chrisdavenport commented Nov 15, 2014

Uh oh!

chrisdavenport commented Dec 7, 2014

Uh oh!

dgrammatiko commented Dec 7, 2014

Uh oh!

chrisdavenport commented Dec 8, 2014

Uh oh!

dgrammatiko commented Dec 8, 2014

Uh oh!

dgrammatiko commented Dec 8, 2014

Uh oh!

smanzi commented Dec 10, 2014

Uh oh!

dgrammatiko commented Dec 10, 2014

Uh oh!

smanzi commented Dec 10, 2014

Uh oh!

smanzi commented Dec 10, 2014

Uh oh!

smanzi commented Dec 10, 2014

Uh oh!

infograf768 commented Dec 10, 2014

Uh oh!

smanzi commented Dec 10, 2014

Uh oh!

infograf768 commented Dec 10, 2014

Uh oh!

smanzi commented Dec 10, 2014

Uh oh!

dgrammatiko commented Dec 10, 2014

Uh oh!

infograf768 commented Dec 10, 2014

Uh oh!

chrisdavenport commented Dec 10, 2014

Uh oh!

dgrammatiko commented Dec 10, 2014

Uh oh!

infograf768 commented Dec 11, 2014

Uh oh!

infograf768 commented Dec 11, 2014

Uh oh!

dgrammatiko commented Dec 11, 2014

Uh oh!

infograf768 commented Dec 11, 2014

Uh oh!

dgrammatiko commented Dec 11, 2014