User tests: Successful: Unsuccessful:
This is a replacement of the mootools script with a jquery equivalent.
The script was taken from here and is providing the same functionality with the old one!
Make sure you have data and you indexed it
1. In admin create a menu for finder
2. In the finders options enable suggestions but disable highlighting (it will load mootools)
3. Try it in the front end
Labels |
Added:
?
|
JavaScript which is MIT licensed should be fine. We already include other such libraries. However, I think the 3rd-party code should be unmodified (we don't want to have to maintain 3rd-party code) and should be placed in the /media directory.
@chrisdavenport It is not modified at all, simple copy paste here! I guess it’s better to use their minified version and not the uncompressed that I included...
@chrisdavenport @infograf768 I did include both scripts (norm, min) and adjust the call to reflect that. Now should be clear that these are unmodified
@chrisdavenport Good catch, updating both
Another thought has just occurred to me. Many people, myself included, will have used template overrides to customise the component and module output. Since the output from suggestions.php is now different, any site using such overrides will break.
In order to maintain backwards compatibility we will need to continue providing the same output for Ajax calls and continue providing the Mootools autocompleter. Consequently, the jQuery version will need to use a different URI for its Ajax calls so that both versions will work.
Good point. I am about to leave my desk now, but I will provide the changes needed for that later tonight
@chrisdavenport I pushed some code with:
1. Keep all the old stuff (controller, model, css and js)
2. I named the new controller and model: suggester (not my brightest moment :) )
3. I added some logger for the model (do we have to include for every method? )
B/C should be solid
I'm seeing a strange problem with the autocompleter behaviour. With the default sample data, I tried entering the search term "the installation". As I type part way through "installation" this brings up two possibilities: "the installation process" and "the installation". However, as soon as I finish typing "the installation", both suggestions disappear! If I then try to type a space it gets removed. If I try to enter the search term "the installation process" it always gets shown as "the installationprocess" and the search fails. Very strange. Seems to happen whenever I try to enter a search term which brings up a suggestion with more than 2 words in it. In other words, it's impossible to search for a 3 word phrase.
The new code also seems to be generating a lot more Ajax requests than the old code. I think it gets called on every keystroke whereas the old code had some sort of timer so a fast typist could not overwhelm it.
Also, could you change the POST to a GET? No point in propagating bad practice into the new code.
Note: Also need to check that it works in an environment with multi-byte characters.
@chrisdavenport Can you test it without this line ?
Which line? Your link takes me to a diff with no particular line highlighted.
line 69 default_form.php mute this: delimiter: /(,|;)\s*/,
No, still not working. :-(
Sorry, I amended the wrong file! Yes, that fixes the issue.
The old code had delay: 1000
but there is a timer here as well so i will set it back to 1 sec.
Also the get comes in the next push.
I have a greek site, somewhere, i’ll try to test it there
I've just been playing with deferRequestBy. I think 1000 seems a bit sluggish and I prefer 500. What do you think?
Yes, 1000 is far too slow, 500 seems perfect to me
Okay, this is working well for me. I'm going to look at code style now. Nearly done, thanks for hanging in there. :-)
@chrisdavenport As I promised I tested this with Greek language and it works like a charm
@test: All good. Well done.
@chrisdavenport You mentioned it should be in the /media folder and @dgt41 has done this but in a subfolder named com_finder. Should it be there or in a more vendor specific folder?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5128.
I vote for /media/jui/js
/media/jui/js sounds good to me.
Sorry, I'm going to throw a small spanner in the works. I think the code can be simplified quite a bit...
By adding "paramName: 'q'" to the autocompleter Ajax call, you can avoid having to create new FinderControllerSuggester and FinderModelSuggester classes. If you add a suggest() method to the existing FinderControllerSuggestions class which does basically the same thing as the existing display() method, except for the slightly different JSON output, then call it using "&task=suggestions.suggest" instead of "&task=suggestions.display", I think that should work.
You can even extract out the common parts of the display() and suggest() methods into an internal protected method in order to avoid the code duplication entirely.
And if you can figure out how to get the autocompleter to use the existing JSON then you can avoid having to add the suggest() method entirely. Not sure if that's possible though, I didn't understand the documentation on that point.
But the script expects
{
“suggestions:”
[
JSON data
]
}
Probably we can alter our output in the client side and add the missing parts… Let’s give it a try
This code works:
public function suggest()
{
$return = array();
$params = JComponentHelper::getParams('com_finder');
if ($params->get('show_autosuggest', 1))
{
// Get the suggestions.
$model = $this->getModel('Suggestions', 'FinderModel');
$return = $model->getItems();
}
// Check the data.
if (empty($return))
{
$return = array();
}
// Use the correct json mime-type
header('Content-Type: application/json');
// Send the response.
echo '{ "suggestions": ' . json_encode($return) . ' }';
JFactory::getApplication()->close();
}
Yes, that's what I was suggesting (no pun intended).
Now, you have two methods, display() and suggest() that are identical except for the last couple of lines, so consider extracting the duplicate code into a separate (protected) method, getSuggestions(), say. So you end up with something like this
public function suggest()
{
$suggestions = $this->getSuggestions();
// Use the correct json mime-type
header('Content-Type: application/json');
// Send the response.
echo '{ "suggestions": ' . json_encode($suggestions) . ' }';
JFactory::getApplication()->close();
}
and similarly for the display() method.
But if you can get the new script to work with the old JSON output, then even better.
Would you like to review, before I push the code:
<?php
/**
* @package Joomla.Site
* @subpackage com_finder
*
* @copyright Copyright (C) 2005 - 2014 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE
*/
defined('_JEXEC') or die;
/**
* Suggestions JSON controller for Finder.
*
* @since 2.5
*/
class FinderControllerSuggestions extends JControllerLegacy
{
/**
* Method to find search query suggestions. Uses jQuery and autocopleter.js
*
* @return void
*
* @since 3.4
*/
public function suggest()
{
$suggestions = $this->getSuggestions();
// Use the correct json mime-type
header('Content-Type: application/json');
// Send the response.
echo '{ "suggestions": ' . json_encode($suggestions) . ' }';
JFactory::getApplication()->close();
}
/**
* Method to find search query suggestions. Uses Mootools and autocompleter.js
*
* @param boolean $cachable If true, the view output will be cached
* @param array $urlparams An array of safe url parameters and their variable types, for valid values see {@link JFilterInput::clean()}.
*
* @return void
*
* @since 2.5
* @deprecated 3.4
*/
public function display($cachable = false, $urlparams = false)
{
$suggestions = $this->getSuggestions();
// Send the response.
echo json_encode($suggestions);
JFactory::getApplication()->close();
}
/**
* Method to retrieve the data from the database
*
* @return array The suggested words
*
* @since 3.4
*/
protected function getSuggestions()
{
$return = array();
$params = JComponentHelper::getParams('com_finder');
if ($params->get('show_autosuggest', 1))
{
// Get the suggestions.
$model = $this->getModel('Suggestions', 'FinderModel');
$return = $model->getItems();
}
// Check the data.
if (empty($return))
{
$return = array();
}
return $return;
}
}
You need to put "header('Content-Type: application/json');" into the display method too.
There's a couple of trivial code style issues, but otherwise looks good.
/media/jui/js sounds good to me too.
@roland-d @chrisdavenport I already put the scripts in media/jui/js. One small consideration: in case that com_finder becomes a candidate for removal, the same story as weblinks, these files should still remain there, otherwise moving them there is really useless. Just a thought...
Category | ⇒ | JavaScript Search |
@test success @dgt41 Very nice job Dimitris!
Have you tried it in a multilingual environment? There used to be a bug that when one language was selected, content flagged as "All" (languages) was not searched. I don't have a multilingual setup now, but as soon as I'll have the time I'll build one and let you know...
@smanzi I think for this responsible is the code in the model, and this PR didn’t touch that. I tested this with greek lang, but only in terms of picking the right words (accents, punctuation etc). Looking at the model and the code used here I can see this:
// Set the query language
if (JLanguageMultilang::isEnabled())
{
$lang = JFactory::getLanguage()->getTag();
}
else
{
$lang = FinderIndexerHelper::getDefaultLanguage();
}
$lang = FinderIndexerHelper::getPrimaryLanguage($lang);
$this->setState('language', $lang);
@dgt41
I'm seeing something strange: suggestion for a term that appears only inside an <h1> tag ("English") is displayed also out of context and concatenated (no space) with other terms.
I'm unsure if this is a problem with the indexer or your suggestion JS.
How can I help? Going on Skype now...
Tested without this PR: same result. So this is not a problem introduced by your PR!
OK, will do later (going to lunch now...) and... there is more not functioning in com_finder (especially in a multilingual environment....)!
@chrisdavenport: I just created issue #5204 for the above and other com_finder issues...
Thanks!
Sergio
Can this be marked as RTC or does it need further testing?
This has three tests... I don't know why it is not RTC yet...
Status | Pending | ⇒ | Ready to Commit |
Moving to RTC, 3 successful tests.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5128.
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-12-13 20:37:58 |
Merged - thanks!
Labels |
Removed:
?
|
I'm unsure if this is a problem with the indexer or your suggestion JS.
My site had the same problem, site https://fortegrp.com/
My site had the same problem, site https://fortegrp.com/
You don't describe what's the problem... Also, this PR was merged 7 years ago and it wasn't meant to solve any other problem apart from the fact that it was dropping Mootools and used jQuery.
@infograf768 @Bakual Any thoughts on including someone else’s code? I used this script https://github.com/devbridge/jQuery-Autocomplete which is MIT licensed.