Disable search field in case there is no search available to the curr… #16810
Conversation
…ent selected app - fixes #14544
|
yep. noticed this too. Please back port 👍 |
| if(self.hasFilter(getCurrentApp())) { | ||
| return; | ||
| } | ||
| if ($searchResults.length === 0) { |
There was a problem hiding this comment.
Why? This is always !== 0, because it is the div with the selector '#searchResults` and if it not exists it gets created. If I remove the if statement it works as intended.
There was a problem hiding this comment.
I was testing this with gallery and $searchResults is empty - I took this as criteria to disable search .... let me have a second look
There was a problem hiding this comment.
aha - gallery has no div for app-content and the search result tag cannot be inserted
Maybe we shall not automatically add that div - if an app implements the search is should add the div explicitly.
@PVince81 @butonic @jancborchardt @MorrisJobke @nickvergessen @oparoz objections?
There was a problem hiding this comment.
Yes, and make the "search results" opt-in instead of opt-out.
|
Which apps can we test this with? |
|
Files should show the search - same for the mail app. |
|
@oparoz Personal/admin settings should not show the search box |
|
OK, so I can confirm that it doesn't work properly when reaching the settings page :) |
|
search is not opt-in - please review and test |
|
settings/apps does not have the search box anymore, although it allowed to filter the long lists. |
damn 👿 |
| .append($searchResults) | ||
| .find('.viewcontainer').css('min-height', 'initial'); | ||
| } | ||
| $searchResults.load(OC.webroot + '/core/search/templates/part.results.html', function () { | ||
| OC.Search = new OCA.Search($('#searchbox'), $('#searchresults')); |
There was a problem hiding this comment.
@PVince81 because the search object here is not instanciated right away the plugins are not all loaded and therefor filtering is not longer working.
What would be the ideal solution to get some kind of execution order into this?
There was a problem hiding this comment.
The non-ideal hacky way:
_.defer(function() { OC.Search = ... }.
I'll need to dig deeper into this.
There was a problem hiding this comment.
I do think the search box should be opt-in (at least in the future).
An app/page should tell whether it needs the search box so it can be shown and trigger events.
Else it's all just guessing / hackery.
There was a problem hiding this comment.
The non-ideal hacky way:
_.defer(function() { OC.Search = ... }.I'll need to dig deeper into this.
THX - works - but I guess this is not deterministic?
|
A new inspection was created. |
|
@nickvergessen fixed - care to take a second look? THX |
|
Works better good now and also resolves #16823 |
|
Good call on not showing the search box when it’s not available. Also serves as a visual reminder that we need to introduce it in some places. 👍 |
|
Removes the field in Gallery+, Settings Seems to work |
|
🚀 Test PASSed.🚀 |
…nted Disable search field in case there is no search available to the curr…
|
@DeepDiver1975 Please create the backport PRs |
…ent selected app - fixes #14544
@oparoz @jancborchardt @nickvergessen @PVince81 @butonic
@karlitschek we have this non working search field in some apps since oc8 - backport is kind of reasonable