Skip to content
Jump to bottom

Improve global search bar and search results page (CFM-220) #59

Merged
merged 2 commits into from
Dec 6, 2022
+422 −191
Merged

Improve global search bar and search results page (CFM-220) #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9a7d5d
Improve global search bar and search results page; fix double-encodin…
arlen Nov 11, 2022
2d4c964
Improve global search result count messages (CFM-220)
arlen Nov 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/resources/locales/en_US/operation.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@ msgstr "Save"
msgid "search"
msgstr "Search"

msgid "search.global"
msgstr "Global Search"

msgid "search.retry"
msgstr "Please select an option from a menu, or try your search again."

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in result.po?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in push 2d4c964

msgid "skip_to_content"
msgstr "Skip to main content"

Expand Down
13 changes: 11 additions & 2 deletions app/resources/locales/en_US/result.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,20 @@ msgstr "Search limit reached"
msgid "search.none"
msgstr "No results found"

msgid "search.result.found"
msgstr "Found: "

msgid "search.result.found.people"
msgstr "{0,plural,=1{1 person} other{{1} people}}"

msgid "search.result.found.groups"
msgstr "{0,plural,=1{1 group} other{{1} groups}}"

Copy link
Contributor

Choose a reason for hiding this comment

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

These localizations violate RTL ("Found: "), don't scale (are we going to have one key per model?) and create duplicate translations (vs controller.po). Unfortunately, it's going to be really hard to properly RTL this construction AND maintain proper pluralization. Instead, I'd suggest switching to a list layout that would render like

Found

  • 39 People
  • 11 Groups

You'll then have two localizations:

search.result.found = "Found"
search.result.found.modelCount = "{0} {1}"

For the latter, you'd build each entry via something like

__d('result', 'search.result.found.modelCount', [$count, __d('controller', "People", [$count])]);

Copy link
Contributor Author

@arlen arlen Nov 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That approach for scaling the models is a great idea. Will do.

The output is in list mode already (CSS is used to output the list inline, comma included) - and we could do the same for the colon on the "Found:" (i.e. use CSS to add it - allowing it to be easily overridden) - or just remove the colon ... which is probably better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in push 2d4c964

msgid "search.result.id"
msgstr "({0})"
msgstr "ID {0}"

msgid "search.result.related"
msgstr "({0}, {1}: {2})"
msgstr "{0}: {1}, ID {2}"

msgid "search.results"
msgstr "Search Results"
19 changes: 14 additions & 5 deletions app/src/Controller/DashboardsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,21 @@ public function search() {

// XXX Still need to implement this (see also CFM-126)
$roles = [];

if(!empty($this->request->getData('q'))
// Only process the request if there are non-space characters
&& !ctype_space($this->request->getData('q'))) {
// Trim leading and trailing whitespace

// Gather our search string.
$q = '';
if(!empty($this->request->getData('global-search-q'))) {
// A search was passed in from the global search-bar.
$q = trim($this->request->getData('global-search-q'));
// Now pass the search string to the in-page search form and empty the global search bar.
$this->setRequest($this->getRequest()->withData('global-search-q', '')->withData('q', $q));
} elseif(!empty($this->request->getData('q'))) {
// A search was passed in from the form on the Global Search page.
$q = trim($this->request->getData('q'));
}

// Only process the request if we have a string of non-space characters
if(!empty($q)) {

// Pull our search configuration
$CoSettings = TableRegistry::getTableLocator()->get('CoSettings');
Expand Down
201 changes: 166 additions & 35 deletions app/templates/Dashboards/search.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,47 +25,178 @@
* @license Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
*/

// Start with the Primary Registry objects
foreach(['People', 'Groups'] as $pm) {
if(!empty($vv_results[$pm])) {
print "<h2>" . __d('controller', $pm, 2) . "</h2>\n";
print "<ul>\n";
$options = [
'type' => 'post',
'url' => [
'plugin' => null,
'controller' => 'dashboards',
'action' => 'search'
],
'id' => 'search'
];

foreach($vv_results[$pm] as $pkey => $matches) {
$url = [
'controller' => \Cake\Utility\Inflector::dasherize($pm),
'action' => 'edit',
$pkey
];
$noResults = true;
?>

// The same entity can match on more than one searchable model
<div class="pageTitleContainer">
<div class="pageTitle">
<h1><?= __d('operation','search.global'); ?></h1>
</div>
</div>

foreach($matches as $m => $entity) {
$displayField = $vv_supported_models[$m]['displayField'];
$displayLabel = __d('field', $displayField);
$displayString = $entity->$displayField;
<div id="search-container" aria-labelledby="global-search-toggle">
<?php
print $this->Form->create(null, $options);
print $this->Form->hidden('co_id', ['default' => $vv_cur_co->id]);
?>
<div class="input-group">
<?php
print $this->Form->label(
'q',
__d('operation','search'),
[
'class' => 'visually-hidden'
]
);
print $this->Form->input(
'q',
[
'id' => 'q',
'class' => 'form-control',
'placeholder' => __d('field','search.placeholder')
]
);
print $this->Form->button(
'<span class="material-icons-outlined">close</span>',
['type' => 'button', 'escapeTitle' => false, 'id' => 'search-clear', 'class' => 'btn btn-link']
);
print $this->Form->button(
__d('operation','search'),
['type' => 'submit', 'escapeTitle' => false, 'class' => 'btn btn-primary btn-sm']
);
?>
</div>
<?php
print $this->Form->end();
?>

// If we match on a related model (for example PersonRoles for People)
// indicate what actually matched
$matchInfo = __d('result', 'search.result.id', $pkey);
<?php /* keep the following temporarily:
<div id="global-search-type">
<div class="form-check form-check-inline">
<input class="form-check-input" type="radio" id="gs-type-basic" name="gs-type">
<label class="form-check-label" for="gs-type-basic">
basic
</label>
</div>
<div class="form-check form-check-inline">
<input class="form-check-input" type="radio" id="gs-type-advanced" name="gs-type">
<label class="form-check-label" for="gs-type-advanced">
advanced
</label>
</div>
</div> */ ?>
</div>

<h2><?= $vv_title; ?></h2>

// Do we have a more informative string to render?
if(!empty($entity->person->primary_name->full_name)) {
$displayString = $entity->person->primary_name->full_name;
<!-- Flash Messages and defined Info Banners -->
<div class="alert-container" id="flash-messages">
<?= $this->Flash->render() ?>

$matchInfo = __d('result', 'search.result.related', $pkey, $displayLabel, $entity->$displayField);
} elseif(!empty($entity->group->name)) {
$displayString = $entity->group->name;
<?php if(!empty($indexBanners)): ?>
<?php foreach($indexBanners as $b): ?>
<?= $this->Alert->alert($b, 'warning') ?>
<?php endforeach; // $indexBanners ?>
<?php endif; // $indexBanners ?>

$matchInfo = __d('result', 'search.result.related', $pkey, $displayLabel, $displayString);
}
<?php if(!empty($banners)): ?>
<?php foreach($banners as $b): ?>
<?= $this->Alert->alert($b, 'warning') ?>
<?php endforeach; // $banners ?>
<?php endif; // $banners ?>
</div>

// XXX This construction isn't ideal, but presumably will get rewritten when the
// design of this page is redone
print "<li>" . $this->Html->link($displayString, $url) . " " . filter_var($matchInfo, FILTER_SANITIZE_SPECIAL_CHARS). "</li>\n";
}
}
<?php
$peopleResultsCount = count($vv_results['People']);
$groupsResultsCount = count($vv_results['Groups']);
?>
<?php if($peopleResultsCount || $groupsResultsCount): ?>
<?php $noResults = false; ?>
<ul id="search-results-meta">
<li class="search-results-found"><?= __d('result', 'search.result.found') ?></li>
<?php if($peopleResultsCount): ?>
<li>
<?= __d('result','search.result.found.people', [$peopleResultsCount,$peopleResultsCount]) ?>
</li>
<?php endif; ?>
<?php if($groupsResultsCount): ?>
<li>
<?= __d('result','search.result.found.groups', [$groupsResultsCount,$groupsResultsCount]) ?>
</li>
<?php endif; ?>
</ul>
<?php endif; ?>

<div id="search-results">
<!-- Start with the Primary Registry objects -->
<?php foreach(['People', 'Groups'] as $pm): ?>
<?php if(!empty($vv_results[$pm])): ?>
<div class="search-results-group-container">
<h3><?= __d('controller', $pm, 2); ?></h3>
<ul class="search-results-group">

<?php foreach($vv_results[$pm] as $pkey => $matches): ?>
<?php
$url = [
'controller' => \Cake\Utility\Inflector::dasherize($pm),
'action' => 'edit',
$pkey
];
// The same entity can match on more than one searchable model
?>

<?php foreach($matches as $m => $entity): ?>
<?php
$displayField = $vv_supported_models[$m]['displayField'];
$displayLabel = __d('field', $displayField);
$displayString = $entity->$displayField;

// If we match on a related model (for example PersonRoles for People)
// indicate what actually matched
$matchInfo = __d('result', 'search.result.id', $pkey);

// Do we have a more informative string to render?
if(!empty($entity->person->primary_name->full_name)) {
$displayString = $entity->person->primary_name->full_name;

$matchInfo = __d('result', 'search.result.related', $displayLabel, $entity->$displayField, $pkey);
} elseif(!empty($entity->group->name)) {
$displayString = $entity->group->name;

$matchInfo = __d('result', 'search.result.related', $displayLabel, $displayString, $pkey);
}
?>
<li class="search-result">
<a href="<?= $this->Url->build($url) ?>">
<div class="search-result-name">
<?= $displayString ?>
</div>
<div class="search-result-match-info">
<?= filter_var($matchInfo, FILTER_SANITIZE_SPECIAL_CHARS) ?>
</div>
</a>
</li>
<?php endforeach; ?>
<?php endforeach; ?>
</ul>
</div>
<?php endif; ?>
<?php endforeach; ?>

print "</ul>\n";
}
}
<?php if($noResults): ?>
<p>
<?= __d('operation','search.retry'); ?>
</p>
<?php endif; ?>

</div>
2 changes: 1 addition & 1 deletion app/templates/element/flash/default.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@
?>

<?php if(!empty($message)): ?>
<?= $this->Alert->alert(h($message), 'warning', true, __d('information','flash.default')) ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to replace this with something else? (h() is the equivalent of htmlspecialchars().) eg Should we be filter_varing this?

Copy link
Contributor Author

@arlen arlen Nov 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(Edited) I don't think so: the latest PHP filters documentation suggests using htmlspecialchars() over a number of filters that are now being deprecated and references it on some we've used that are not being deprecated. The implication seemed to be to favor htmlspecialchars() generally. Since Cake seems to favor this approach as well, it's reasonable to use this for most cases of output escaping. htmlspecialchars is a near equivalent to FILTER_SANITIZE_FULL_SPECIAL_CHARS - which is somewhat more aggressive than the FILTER_SANITIZE_SPECIAL_CHARS we've been using previously.

I'm somewhat on the fence on this - because I do like the verbosity and standardization on filter_var - but I think the h() approach is probably simpler and easier to read.

Copy link
Contributor Author

@arlen arlen Nov 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note also that the messages are being output escaped prior to $this->Alert->alert($message,... which is how they ended up double-encoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave this unchanged for now, but I'm happy to discuss and/or be persuaded. As noted, I'm a little on the fence with this, but h() (or even htmlspecialchars()) seems a better way to go except in special cases (such as FILTER_SANITIZE_URL).

<?= $this->Alert->alert($message, 'warning', true, __d('information','flash.default')) ?>
<?php endif; ?>

2 changes: 1 addition & 1 deletion app/templates/element/flash/error.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
*/ ?>

<?php if(!empty($message)): ?>
<?= $this->Alert->alert(h($message), 'danger', true, __d('information','flash.error')) ?>
<?= $this->Alert->alert($message, 'danger', true, __d('information','flash.error')) ?>
<?php endif; ?>
4 changes: 3 additions & 1 deletion app/templates/element/flash/information.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,7 @@
?>

<?php if(!empty($message)): ?>
<?= $this->Alert->alert(h($message), 'information', true, __d('information','flash.information')) ?>
<?php /* Note: unlike Notice, Error, and Success messages, Information messages require
no prefix. That is, we don't include "Information: " in front of the message. */ ?>
<?= $this->Alert->alert($message, 'information', true) ?>
<?php endif; ?>
2 changes: 1 addition & 1 deletion app/templates/element/flash/success.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
?>

<?php if(!empty($message)): ?>
<?= $this->Alert->alert(h($message), 'success', true, __d('information','flash.success')) ?>
<?= $this->Alert->alert($message, 'success', true, __d('information','flash.success')) ?>
<?php endif; ?>
29 changes: 26 additions & 3 deletions app/templates/element/javascript.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,32 @@

// END DESKTOP MENU DRAWER BEHAVIOR

// GLOBAL SEARCH
$('#search-bar input').focus(function() {
$('#search-bar button').addClass('visible');
// SEARCH
// Persistent search bar form:
$('#global-search form').submit(function () {
// Disallow submit on blank
if($.trim($('#global-search-q').val()) == '') {
return false;
}
});
// Select search text on focus
$('#global-search-q').focus(function() {
$('#global-search-q').select();
});

// Search page form:
$('#search').submit(function () {
// Disallow submit on blank
if($.trim($('#q').val()) == '') {
return false;
}
});
$('#search-clear').click(function () {
$('#q').val('');
$('#q').focus();
});
$('#q').focus(function() {
$('#q').select();
});

// TOP FILTER FORM
Expand Down
33 changes: 23 additions & 10 deletions app/templates/element/searchGlobal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,29 @@
'plugin' => null,
'controller' => 'dashboards',
'action' => 'search'
]
],
'id' => 'global-search-form'
];

print $this->Form->create(null, $options);
print $this->Form->hidden('co_id', ['default' => $vv_cur_co->id]);
print $this->Form->label('q', __d('field','search.placeholder'), ['class' => 'visually-hidden']);
print $this->Form->input('q',['id' => 'q','placeholder' => __d('field','search.placeholder')]);
print $this->Form->button(
__d('operation','search'),
['type' => 'submit', 'escapeTitle' => false, 'class' => 'btn btn-primary']
);
print $this->Form->end();
?>

<button id="global-search-toggle" class="dropdown-toggle top-menu-button" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
<em class="material-icons">search</em>
<span class="visually-hidden"><?= __d('operation','search') ?></span>
</button>

<div id="global-search" class="dropdown-menu" aria-labelledby="global-search-toggle">
<?php
print $this->Form->create(null, $options);
print $this->Form->hidden('co_id', ['default' => $vv_cur_co->id]);
print $this->Form->label('global-search-q', __d('field','search.placeholder'), ['class' => 'visually-hidden']);
print $this->Form->input('global-search-q',['id' => 'global-search-q', 'placeholder' => __d('field','search.placeholder')]);
print $this->Form->button(
'<em class="material-icons">search</em>',
['type' => 'submit', 'escapeTitle' => false, 'id' => 'global-search-button', 'class' => 'btn btn-link btn-sm']
);
print $this->Form->end();
?>
</div>


Loading