Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#33441] fix SmartSearch menu item missing parameters #3285

Closed
wants to merge 13 commits into from

Conversation

itbra
Copy link
Contributor

@itbra itbra commented Mar 12, 2014

Fixing issue described in bug #33458, related to missing menu item parameters in FinderViewSearch results page (components/com_finder/views/search/tmpl/default_results.php) not allowing to enable/disable the display of the related content.

SmartSearch evaluates 2 parameters - show_suggested_query and show_explained_query on the results page (/components/com_finder/views/search/tmpl/default_results.php) that haven't been configurable through the menu item configuration since its deployment through jXtended. But it should be configurable whether or not to suggest alternative search terms or explain how to improve the search rather than overriding the view just for this circumstance.
So i added these two options to the menu item configuration file (/components/com_finder/views/search/tmpl/default.xml) and added translation to (/administrator/language/en-GB/en-GB.com_finder.sys) thus enabling the evaluation of these parameters to do what a parameter should do: enable/disable.
SmartSearch evaluates 2 parameters - show_suggested_query and show_explained_query on the results page (/components/com_finder/views/search/tmpl/default_results.php) that haven't been configurable through the menu item configuration since its deployment through jXtended. But it should be configurable whether or not to suggest alternative search terms or explain how to improve the search rather than overriding the view just for this circumstance.
So i added these two options to the menu item configuration file (/components/com_finder/views/search/tmpl/default.xml) and added translation to (/administrator/language/en-GB/en-GB.com_finder.sys) thus enabling the evaluation of these parameters to do what a parameter should do: enable/disable.
@chrisdavenport
Copy link
Contributor

Could these two parameters be added to the component options too?

@itbra
Copy link
Contributor Author

itbra commented Mar 24, 2014

They are. Check the "files changed" tab. The fix covers administrator/language/en-GB/en-GB.com_finder.sys.ini and components/com_finder/views/search/tmpl/default.xml.

Or mabe i am getting you wrong and you could specify your comment, please?

@chrisdavenport
Copy link
Contributor

I mean when you go Components -> Smart Search -> Options. I think you just need to add the fields into the config.xml file if memory serves.

@itbra
Copy link
Contributor Author

itbra commented Mar 24, 2014

I think this doesn't relate to the component config, because these options related to the view (menu item config) rather than to the component globally. Because, if for example you have more than one search page it might be wished to show them on one page while not wanting to show them on the other. A global option wouldn't allow this and make them on/off-configurable only which makes no sense to me. The menu item options instead allow for different configuration.
But i'm open for your arguments.

@Bakual
Copy link
Contributor

Bakual commented Mar 24, 2014

@itbra What we usually do is adding the parameter in both places. In the menu item we then use an additional (default) option <option value="">JGLOBAL_USE_GLOBAL</option>.
This way you can set a behavior in the component options, and then you can override it in a menu item if needed.
The global one is especially helpful since you don't necessary have a menu item for the search.

So I'd support the request of Chris to add them to the component config plus adding the "Use Global" option to the menu item.

@itbra
Copy link
Contributor Author

itbra commented Mar 24, 2014

Your arguments make sense. So far i didn't think of setting up a global config to be overridden if required.

The global one is especially helpful since you don't necessary have a menu item for the search.

Didn't consider this. In this case it absolutely makes sense. I'll add the requested change asap.

Added *JGLOBAL_USE_GLOBAL* option to *show_suggested_query* and *show_explained_query parameters*
Added *show_suggested_query* and *show_explained_query* parameters to make them configurable and overwritable
@itbra
Copy link
Contributor Author

itbra commented Mar 24, 2014

Done. Please let me know if this suits.

<field name="show_suggested_query"
type="radio"
class="btn-group btn-group-yesno"
default=""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default should be "1" to correspond with current behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@chrisdavenport
Copy link
Contributor

Good work. Just needs some tweaks to the English as indicated.

@@ -124,6 +124,28 @@
<option value="1">JYES</option>
<option value="0">JNO</option>
</field>
<field name="show_suggested_query"
type="radio"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps change the fields to type="list" for consistency with other fields on the screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Changed default values of *show_suggested_query* and *show_explained_query* to 1.
Changed option titles from JYES and JNO to JSHOW and JHIDE
Changed translations for *COM_FINDER_SHOW_SUGGESTED_QUERY_* and *COM_FINDER_SHOW_EXPLAINED_QUERY_*
Changed field types of *show_suggested_query* and *show_explained_query* from radio to list
@@ -34,6 +34,10 @@ COM_FINDER_CONFIG_SHOW_FEED_TEXT_DESC="Show the associated text with the feed, o
COM_FINDER_SELECT_SEARCH_FILTER="Select filter"
COM_FINDER_ALLOW_EMPTY_QUERY_LABEL="Allow Empty Search"
COM_FINDER_ALLOW_EMPTY_QUERY_DESC="Only if a filter is selected, allow an empty search string to initiate a search within the filter constraints."
COM_FINDER_SHOW_SUGGESTED_QUERY_LABEL="Did you mean?"
COM_FINDER_SHOW_SUGGESTED_QUERY_DESC="Suggest alternative search terms when a search produces no results?"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you drop the question mark at the end of this sentence. My fault, I wasn't clear enough in my last comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@chrisdavenport
Copy link
Contributor

With the last couple of tweaks I think it's good to go. Many thanks for working on this.

@infograf768
Copy link
Member

@chrisdavenport

I just checked both existing com.finder ini and sys.ini and found out that most lang strings in the sys.ini should be in fact in the .ini file, (including the new ones in this PR which constants should rather be of the type COM_FINDER_CONFIG_)

My intention was to correct this ASAP and adapt this PR to fit.

As far as I know, we should only keep in the sys.ini:

`; Joomla! Project
; Copyright (C) 2005 - 2014 Open Source Matters. All rights reserved.
; License GNU General Public License version 2 or later; see LICENSE.txt, see LICENSE.php
; Note : All ini files need to be saved as UTF-8

COM_FINDER="Smart Search"
COM_FINDER_XML_DESCRIPTION="Smart Search"

COM_FINDER_MENU_SEARCH_VIEW_DEFAULT_TEXT="The default search layout."
COM_FINDER_MENU_SEARCH_VIEW_DEFAULT_TITLE="Search"
`

Removed question marks
@itbra
Copy link
Contributor Author

itbra commented Mar 31, 2014

True. I was also wondering why these appear in the .sys.ini, but didn't wanna touch it since i thought there was good reason for it. Let me know if you want me to move them.

@infograf768
Copy link
Member

I can make a PR, but it would have to be applied before yours or after yours.
Whatever the decision, please change your constants to use COM_FINDER_CONFIG_ and move them alphabetically to the .ini file instead of the sys.ini

For example
+COM_FINDER_SHOW_SUGGESTED_QUERY_LABEL="Did you mean"
should be
+COM_FINDER_CONFIG_SHOW_SUGGESTED_QUERY_LABEL="Did you mean"

and evidently change in the xml(s)

My test show here that all COM_FINDER_CONFIG_etc strings in the sys.ini should be deleted.

Also the
COM_FINDER_SEARCH_FILTER_SEARCH_LABEL="Search Filter"
COM_FINDER_SEARCH_FILTER_SEARCH_DESC="Selecting a Search Filter will limit any searches submitted to use the selected filter."
COM_FINDER_SEARCH_SEARCH_QUERY_LABEL="Search Query"
COM_FINDER_SEARCH_SEARCH_QUERY_DESC="Entering search terms will make this menu item automatically return the results for the predefined terms."

Should be moved to the .ini (alpha the strings when doing such)

@chrisdavenport
Copy link
Contributor

@infograf768 Sounds good to me.
@itbra Please make those changes if you have time. Thanks.

I've noticed some inconsistencies in the UI between the component options and the menu options, but given that we need to get 3.3 out the door, I think it best to get this PR merged and then work on a separate PR to improve the UI.

Added substring *CONFIG_* to new translation keys.
Added substring *CONFIG_* to new options
Added substring *CONFIG_* to new language keys.
@itbra
Copy link
Contributor Author

itbra commented Apr 1, 2014

Done, hope i didn't oversee anything. Please let me know.

@infograf768
Copy link
Member

You forgot to add your new ini strings to /administrator/language/en-GB/en-GB.com_finder.ini instead of the sys.ini

Also, you need to move from the sys.ini to the ini:

COM_FINDER_SEARCH_FILTER_SEARCH_LABEL="Search Filter"
COM_FINDER_SEARCH_FILTER_SEARCH_DESC="Selecting a Search Filter will limit any searches submitted to use the selected filter."
COM_FINDER_SEARCH_SEARCH_QUERY_LABEL="Search Query"
COM_FINDER_SEARCH_SEARCH_QUERY_DESC="Entering search terms will make this menu item automatically return the results for the predefined terms."

And last, to delete from the sys.ini all the existing strings of type COM_FINDER_CONFIG_etc

@itbra
Copy link
Contributor Author

itbra commented Apr 1, 2014

This is quite confusing. You said, you'll to integrate the move of all strings that do not belong to my PR from .sys.ini to .ini via a new PR, which is why i only edited my language strings. Those you mention above are not part of my PR.

Now i don't know what to move where. Just to get things clear: You want me to move all langage strings from .sys.ini to .ini and leave within .sys.ini only this block:

`; Joomla! Project
; Copyright (C) 2005 - 2014 Open Source Matters. All rights reserved.
; License GNU General Public License version 2 or later; see LICENSE.txt, see LICENSE.php
; Note : All ini files need to be saved as UTF-8

COM_FINDER="Smart Search"
COM_FINDER_XML_DESCRIPTION="Smart Search"

COM_FINDER_MENU_SEARCH_VIEW_DEFAULT_TEXT="The default search layout."
COM_FINDER_MENU_SEARCH_VIEW_DEFAULT_TITLE="Search"

@infograf768
Copy link
Member

Leave only in sys.ini these 4 strings indeed, BUT NOT MOVE ALL sys.ini strings to ini.
ONLY move there:

  1. you new strings
    • these older strings
      COM_FINDER_SEARCH_FILTER_SEARCH_LABEL="Search Filter"
      COM_FINDER_SEARCH_FILTER_SEARCH_DESC="Selecting a Search Filter will limit any searches submitted to use the selected filter."
      COM_FINDER_SEARCH_SEARCH_QUERY_LABEL="Search Query"
      COM_FINDER_SEARCH_SEARCH_QUERY_DESC="Entering search terms will make this menu item automatically return the results for the predefined terms."

@itbra
Copy link
Contributor Author

itbra commented Apr 1, 2014

Done.

@infograf768
Copy link
Member

Merged after deleting the redundant CONFIG strings in sys.ini. Thanks

infograf768 added a commit that referenced this pull request Apr 2, 2014
Bakual pushed a commit to Bakual/joomla-cms that referenced this pull request May 12, 2014
Bakual pushed a commit to Bakual/joomla-cms that referenced this pull request May 12, 2014
@piotr-cz
Copy link
Contributor

It's also missing the number of results to show, however functionality to set it is not implemented in Smart search component and falls back to global Default List Limit

@piotr-cz
Copy link
Contributor

There is one more parameters missing in both Component conffig and View config: show_search_form.

@zero-24
Copy link
Contributor

zero-24 commented Sep 28, 2016

Please open a new bug report with all needed information. Comments on a closed / merged PR get missed fast. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants