? ? Success

User tests: Successful: Unsuccessful:

avatar smz
smz
18 Dec 2014

The issue:

When the "Expand Advanced Search" option is selected, the Advanced Search form should be displayed as expanded: this does not happens.

The fix:

Add the in class to the advancedSearch <div> when the option is selected

More issues NOT SOLVED by this PR

  • The JS code for toggling the Advanaced Search form is not functional and can be eliminated (the functionality is guaranteed by BS data-toggle)

  • The JS that should disable fields not used in the search when submitting the form (and thus clean-up the search URL) is not working and search URLs are littered by unused options (e.g.: &Search=&t[]=&t[]=&t[]=)

P.S.: The above issues have been solved too

avatar smz smz - open - 18 Dec 2014
avatar jissues-bot jissues-bot - change - 18 Dec 2014
Labels Added: ?
avatar smz
smz - comment - 18 Dec 2014

The first outstanding issue is trivial: just delete the associated code.

The second one is more tricky and out of my reach: I think it is more @dgt41 territory...

As the second outstanding issue is definitely more important and more difficult to solve, anyone taking it into his hands should feel free to open a new PR and incorporate my fix for the lesser issue into it.

I didn't touch the associated JS (not even by deleting the useless code) so that we can start with a clean situation.

avatar smz smz - change - 18 Dec 2014
Title
Smart Search: Fixes "Expand Advanced Search" option
Issues in Smart Search "Advanced Search"
avatar smz smz - change - 18 Dec 2014
The description was changed
avatar smz
smz - comment - 18 Dec 2014

EASY! Wrong target div name!! Commit is on its way... :smile:

avatar smz
smz - comment - 18 Dec 2014

With the latest commit this PR solves also the "outstanding" issues.
Ready to test.

avatar smz
smz - comment - 18 Dec 2014

... there is something I still don't like: all select boxes have name="t[]"

I don't think this is correct, but it is something that must probably be searched in JHtml::_('filter.select')

And also the &search= at the end of the URL if none of the select boxes are used should be eliminated...

avatar smz
smz - comment - 18 Dec 2014

probably not: all the t[], if I get it right, are for building a taxonomy array and as far as regard the empty &search=... WTF!

Go for testing!! :+1:

avatar dgt41
dgt41 - comment - 18 Dec 2014

@smz Right now I am on the move and cannot spare some time for this and next week is kinda holidays… Anyway just seeing what you’ve done here I have to warn you that all this code you removed is probably the same from bootstrap slider and it shouldn’t be removed but fixed. The idea behind this, its that this affects front end and some templates may not use BS so in that case the front end won’t be functional.
To cut it sort: don’t depend on BS code (esp for front end) as that my not be available!

avatar smz
smz - comment - 18 Dec 2014

@dgt41 Dimitris, the code I removed wasn't working since the times when someone decided that advancedSearch is a nicer ID than advanced-search. That happened in 2012 (August 16), when Joomla was "bootstrapped". I seriously doubt we are going to break B/C by removing that broken code, but admitting there is a 0.0001% probability of breaking a site, I firmly think that B/C cannot be guaranteed when it is based on existing bugs.

Anyone not using BS in front-end has the responsibility to set-up his working framework and the needed overrides with associated JS. We cannot throw random (and not working) JS in our views assuming that this may be useful in different scenarios based on who-knows-what framework and views.

avatar smz
smz - comment - 18 Dec 2014

@dgt41 ... and "fixing" that code by adapting it to the correct target ID will in any case break that 0.0001% of (totally hypothetical) sites who are still using a non-BS framework.

But even more, if they are still using the old advanced-search ID in their views (condition needed for the current JS is working for them), that means they are using an overridden view and thus this mod will not "reach" them.

Conclusion: 100% B/C

avatar smz smz - change - 18 Dec 2014
The description was changed
avatar smz
smz - comment - 18 Dec 2014

Found a new issue and fixed that too: in the "Result Description" dates conditions ("before", "after" and "exact") were not translated.

avatar smz
smz - comment - 18 Dec 2014

Sorry if this became an "omnibus" PR, but it is a case of serendipity in finding new issues starting from a single tiny one...

To recap, for testers, this PR now fixes the following:

  • The "Expand Advanced Search" option is now honored
  • Some broken and useless JS for toggling the (old) "Advanced Search" slider has been removed
  • In absence of advanced search options the returned URL is fixed from &Search=&t[]=&t[]=&t[]= to just &Search= (TODO: remove also that remnant? A router job, I guess...)
  • The "before", "after" and "exact" keywords used to qualify search dates in the "Search description" are now translatable using the following strings:
COM_FINDER_QUERY_DATE_CONDITION_AFTER="after"
COM_FINDER_QUERY_DATE_CONDITION_BEFORE="before"
COM_FINDER_QUERY_DATE_CONDITION_EXACT="exactly on"
avatar brianteeman brianteeman - change - 18 Dec 2014
Category Search
avatar dgt41
dgt41 - comment - 18 Dec 2014

@smz I see it as a wiser choice to leave the script as is and just add the missing -. Please DON’T introduce Bootstrap dependency here. Though you can make an override for protostar with bootstrap code.

avatar smz
smz - comment - 18 Dec 2014

@dgt41 I'm afraid you're missing the point: the bloody thing is (and was) already BS. I didn't change anything to make it more "BS dependent". Maybe you don't have the code at hand, but, when you can, give it look and also try to comment out the slider JS: "wow! slider still working... WTF!" (that was my reaction when I did it while trying to fix the "Expand Advanced Search" option into that piece of code...). I wasted hours into the damn JS trying to figure out HOW it could work, and at the end... it simply wasn't! Since 2012.

What I would like to do with this PR is to have things working, now, for 3.4.0. I'm not breaking anything and I'm fixing 3 bugs.

If you think that in the future we should be less BS dependent, I can agree with you and when we will move all of this to layouts we can make wonders for that, but IMHO this is not the timing.

avatar dgt41
dgt41 - comment - 18 Dec 2014

@smz if you just add the - without touching the script, the problem is gone? If yes then just do that, and everybody will be happy!

avatar smz
smz - comment - 18 Dec 2014

No, Dimitris, because the JS is also handling the cleanup for the unused advanced fields (and hence the URL).

To fix it I should modify the script by changing the target and at that point we will have two colliding JS functions (the BS and "ours") handling the slider.

If I instead just fix the target only in the "fields cleanup" part of it and leave the slider JS as it is we will have a JS targeting a non-existent slider. What will be the sense in that?

avatar dgt41
dgt41 - comment - 18 Dec 2014

OK I just stated that this is the wrong way (my opinion). You introduce BS dependency and therefore this cannot be undone before v4.0
Again for me this shouldn't be fixed this way, we should do it properly: css framework free

avatar smz
smz - comment - 18 Dec 2014

@dgt41 BTW, I just tried to follow your suggestion and just added the - to the advanced search <div>: as expected nothing works anymore.

That JS is simply a left-over of the times we had a totally different HTML structure and must be adjusted to the current one.

As far as regards the "slider toggling" part of it adjusting is removing.

avatar smz
smz - comment - 18 Dec 2014

Dimitris: I A M N O T I N T R O D U C I N G B S D E P E N D E N C Y
it is still was already there! :smile:

avatar smz
smz - comment - 18 Dec 2014

Historically, BS dependency, here, has been introduced on 2012 August 16, by @kyleledbetter with his commit cd663c0 titled (guess what?) "Bootstrap the site."

avatar stellainformatica
stellainformatica - comment - 21 Dec 2014

I've tested the patch with the staging, but I see only the suggestions, not the advanced search fields (see attachment) screen shot 2014-12-21 at 09 48 19


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

avatar stellainformatica stellainformatica - test_item - 21 Dec 2014 - Tested unsuccessfully
avatar smz
smz - comment - 21 Dec 2014

That's strange... I have it in my test site (current staging):
capture

Just to be sure: do you have articles in your site, have published all the "Smart Search" plugins and performed an "Index" in "Smart Search" backend?

avatar chrisdavenport
chrisdavenport - comment - 21 Dec 2014

@stellainformatica Do you have advanced search enabled in the options? I think it's disabled by default.

avatar stellainformatica stellainformatica - test_item - 21 Dec 2014 - Tested successfully
avatar stellainformatica
stellainformatica - comment - 21 Dec 2014

Sorry, I forgot to make the Index before testing :P
After making the Index the search fields correctly appear.
I have tested with the default template Protostar with Bootstrap


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

avatar smz
smz - comment - 21 Dec 2014

:+1:

avatar smz
smz - comment - 30 Dec 2014

@chrisdavenport We still have this open for com_finder...

avatar anibalsanchez anibalsanchez - test_item - 15 Jan 2015 - Tested successfully
avatar anibalsanchez
anibalsanchez - comment - 15 Jan 2015

@test OK

After patch, it works fine


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

avatar zero-24 zero-24 - change - 15 Jan 2015
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 15 Jan 2015

Thanks for testing, commenting and coding -> RTC


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

avatar brianteeman brianteeman - change - 16 Jan 2015
Labels Added: ?
avatar zero-24 zero-24 - close - 17 Jan 2015
avatar roland-d roland-d - close - 17 Jan 2015
avatar roland-d roland-d - change - 17 Jan 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-01-17 09:13:21
avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 14 Oct 2015
Labels Added: ?

Add a Comment

Login with GitHub to post a comment