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

Improve search (make fuzzy again and more robust) #932

Merged
merged 12 commits into from
Nov 4, 2024
10 changes: 7 additions & 3 deletions lib/features/popup-menu/PopupMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import PopupMenuComponent from './PopupMenuComponent';
/**
* @typedef {import('../../core/Canvas').default} Canvas
* @typedef {import('../../core/EventBus').default} EventBus
* @typedef {import('../search/search').default} search
*
* @typedef {import('../../util/Types').Point} Point
*
Expand Down Expand Up @@ -59,10 +60,12 @@ var DEFAULT_PRIORITY = 1000;
* @param {PopupMenuConfig} config
* @param {EventBus} eventBus
* @param {Canvas} canvas
* @param {search} search
*/
export default function PopupMenu(config, eventBus, canvas) {
export default function PopupMenu(config, eventBus, canvas, search) {
this._eventBus = eventBus;
this._canvas = canvas;
this._search = search;

this._current = null;

Expand Down Expand Up @@ -94,7 +97,8 @@ export default function PopupMenu(config, eventBus, canvas) {
PopupMenu.$inject = [
'config.popupMenu',
'eventBus',
'canvas'
'canvas',
'search'
];

PopupMenu.prototype._render = function() {
Expand Down Expand Up @@ -138,6 +142,7 @@ PopupMenu.prototype._render = function() {
scale=${ scale }
onOpened=${ this._onOpened.bind(this) }
onClosed=${ this._onClosed.bind(this) }
searchFn=${ this._search }
...${{ ...options }}
/>
`,
Expand Down Expand Up @@ -550,7 +555,6 @@ PopupMenu.prototype._getHeaderEntries = function(target, providers) {


PopupMenu.prototype._getEmptyPlaceholder = function(providers) {

const provider = providers.find(
provider => isFunction(provider.getEmptyPlaceholder)
);
Expand Down
36 changes: 15 additions & 21 deletions lib/features/popup-menu/PopupMenuComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { isDefined, isFunction } from 'min-dash';
* @typedef {import('./PopupMenuProvider').PopupMenuHeaderEntry} PopupMenuHeaderEntry
* @typedef {import('./PopupMenuProvider').PopupMenuEmptyPlaceholderProvider | import('./PopupMenuProvider').PopupMenuEmptyPlaceholder} PopupMenuEmptyPlaceholder
*
* @typedef {import('../search/search').default} search
*
* @typedef {import('../../util/Types').Point} Point
*/

Expand All @@ -41,6 +43,7 @@ import { isDefined, isFunction } from 'min-dash';
* @param {boolean} [props.search]
* @param {PopupMenuEmptyPlaceholder} [props.emptyPlaceholder]
* @param {number} [props.width]
* @param {search} props.searchFn
*/
export default function PopupMenuComponent(props) {
const {
Expand All @@ -54,6 +57,7 @@ export default function PopupMenuComponent(props) {
scale,
search,
emptyPlaceholder,
searchFn,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have some type definition for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also missing here, good catch.

Copy link
Member Author

@nikku nikku Nov 4, 2024

Choose a reason for hiding this comment

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

Fixed via f1a8efe.

entries: originalEntries,
onOpened,
onClosed
Expand All @@ -75,29 +79,19 @@ export default function PopupMenuComponent(props) {
return originalEntries;
}

const filter = entry => {
if (!value) {
return (entry.rank || 0) >= 0;
}

if (entry.searchable === false) {
return false;
}
if (!value) {
return originalEntries.filter(({ rank = 0 }) => rank >= 0);
}

const searchableFields = [
entry.description || '',
entry.label || '',
entry.search || ''
].map(string => string.toLowerCase());

// every word of `value` should be included in one of the searchable fields
return value
.toLowerCase()
.split(/\s/g)
.every(word => searchableFields.some(field => field.includes(word)));
};
const searchableEntries = originalEntries.filter(({ searchable }) => searchable !== false);

return originalEntries.filter(filter);
return searchFn(searchableEntries, value, {
keys: [
'label',
'description',
'search'
]
}).map(({ item }) => item);
}, [ searchable ]);

const [ entries, setEntries ] = useState(filterEntries(originalEntries, value));
Expand Down
3 changes: 3 additions & 0 deletions lib/features/popup-menu/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import PopupMenu from './PopupMenu';

import Search from '../search';


/**
* @type { import('didi').ModuleDeclaration }
*/
export default {
__depends__: [ Search ],
__init__: [ 'popupMenu' ],
popupMenu: [ 'type', PopupMenu ]
};
9 changes: 6 additions & 3 deletions lib/features/search-pad/SearchPad.js
Original file line number Diff line number Diff line change
Expand Up @@ -523,10 +523,13 @@ function createHtmlText(tokens) {
var htmlText = '';

tokens.forEach(function(t) {
if (t.matched) {
htmlText += '<b class="' + SearchPad.RESULT_HIGHLIGHT_CLASS + '">' + escapeHTML(t.matched) + '</b>';
var text = escapeHTML(t.value || t.matched || t.normal);
var match = t.match || t.matched;

if (match) {
htmlText += '<b class="' + SearchPad.RESULT_HIGHLIGHT_CLASS + '">' + text + '</b>';
} else {
htmlText += escapeHTML(t.normal);
Copy link
Contributor

Choose a reason for hiding this comment

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

so the normal property is not used anymore? What was the purpose of it before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that is a bug. 👍

Of course normal is still used, but we did not properly test cover it to date.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bug fix: 32d8f0e

Test coverage added for rendering (different cases): 1fd6df6

htmlText += text;
}
});

Expand Down
9 changes: 8 additions & 1 deletion lib/features/search-pad/SearchPadProvider.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import type { Element } from '../../model/Types';

export type Token = {
type LegacyToken = {
matched?: string;
normal?: string;
};

type ModernToken = {
nikku marked this conversation as resolved.
Show resolved Hide resolved
match: boolean;
value: string;
};

export type Token = LegacyToken | ModernToken;

export type SearchResult = {
primaryTokens: Token[];
secondaryTokens: Token[];
Expand Down
8 changes: 8 additions & 0 deletions lib/features/search-pad/SearchProvider.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,20 @@ export class FooSearchProvider implements SearchProvider {
{
matched: pattern,
normal: pattern
},
{
match: true,
value: 'bar'
}
],
secondaryTokens: [
{
matched: 'bar',
normal: 'bar'
},
{
match: false,
value: 'foo'
}
],
element: create('shape')
Expand Down
Loading