From db91d3141677bff4b3e2c7b92921965fe9cba95c Mon Sep 17 00:00:00 2001 From: Cyprien P Date: Wed, 19 May 2021 09:07:51 +0200 Subject: [PATCH] (core) Search improvemement Summary: This diff implements the search improvement that are mentioned here https://grist.quip.com/j1biAmfGkbzV/Search-Improvements. CAVEATS: I've noticed a bit of a usability caveats: the tooltips overlap with the new `search all pages` checkbox, which requires user to move cursor away for a bit in order to be able to click the button. {F28224} I've experimented locally with tooltips showing on both sides of the arrows, but it overlaps with the cross icon so could also be an issue. I couldn't think of any clear simple alternative, probably not too big of an issue anyway. Test Plan: Added new test. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2818 --- app/client/models/SearchModel.ts | 30 ++++++++- app/client/ui2018/search.ts | 101 ++++++++++++++++++++++++------- 2 files changed, 107 insertions(+), 24 deletions(-) diff --git a/app/client/models/SearchModel.ts b/app/client/models/SearchModel.ts index ef0d0868..60016a62 100644 --- a/app/client/models/SearchModel.ts +++ b/app/client/models/SearchModel.ts @@ -17,7 +17,9 @@ export interface SearchModel { value: Observable; // string in the search input isOpen: Observable; // indicates whether the search bar is expanded to show the input noMatch: Observable; // indicates if there are no search matches + isEmpty: Observable; // indicates whether the value is empty isRunning: Observable; // indicates that matching is in progress + multiPage: Observable; // if true will search across all pages findNext(): Promise; // find next match findPrev(): Promise; // find previous match @@ -69,6 +71,8 @@ export class SearchModelImpl extends Disposable implements SearchModel { public readonly isOpen = Observable.create(this, false); public readonly isRunning = Observable.create(this, false); public readonly noMatch = Observable.create(this, true); + public readonly isEmpty = Observable.create(this, true); + public readonly multiPage = Observable.create(this, false); private _searchRegexp: RegExp; private _tabStepper = new Stepper(); private _sectionStepper = new Stepper(); @@ -78,6 +82,7 @@ export class SearchModelImpl extends Disposable implements SearchModel { private _fieldFormatters: BaseFormatter[]; private _startPosition: SearchPosition; private _tabsSwitched: number = 0; + private _isRestartNeeded: boolean = false; constructor(private _gristDoc: GristDoc) { super(); @@ -85,10 +90,21 @@ export class SearchModelImpl extends Disposable implements SearchModel { // Listen to input value changes (debounced) to activate searching. const findFirst = debounce((_value: string) => this._findFirst(_value), 100); this.autoDispose(this.value.addListener(v => { void findFirst(v); })); + + // Set this.noMatch to false when multiPage gets turned ON. + this.autoDispose(this.multiPage.addListener(v => { if (v) { this.noMatch.set(false); }})); + + // Schedule a search restart when user changes pages (otherwise search would resume from the + // previous page that is not shown anymore). Also revert noMatch flag when in single page mode. + this.autoDispose(this._gristDoc.activeViewId.addListener(() => { + if (!this.multiPage.get()) { this.noMatch.set(false); } + this._isRestartNeeded = true; + })); } public async findNext() { if (this.noMatch.get()) { return; } + if (this._isRestartNeeded) { return this._findFirst(this.value.get()); } this._startPosition = this._getCurrentPosition(); await this._nextField(1); return this._matchNext(1); @@ -96,12 +112,15 @@ export class SearchModelImpl extends Disposable implements SearchModel { public async findPrev() { if (this.noMatch.get()) { return; } + if (this._isRestartNeeded) { return this._findFirst(this.value.get()); } this._startPosition = this._getCurrentPosition(); await this._nextField(-1); return this._matchNext(-1); } private _findFirst(value: string) { + this._isRestartNeeded = false; + this.isEmpty.set(!value); if (!value) { this.noMatch.set(true); return; } this._searchRegexp = makeRegexp(value); const tabs: any[] = this._gristDoc.docModel.allDocPages.peek(); @@ -155,7 +174,7 @@ export class SearchModelImpl extends Disposable implements SearchModel { if (p) { await p; } // Detect when we get back to the start position; this is where we break on no match. - if (this._isCurrentPosition(this._startPosition)) { + if (this._isCurrentPosition(this._startPosition) && !this._matches()) { console.log("SearchBar: reached start position without finding anything"); this.noMatch.set(true); return; @@ -256,6 +275,7 @@ export class SearchModelImpl extends Disposable implements SearchModel { } private async _nextTab(step: number) { + if (!this.multiPage.get()) { return; } await this._tabStepper.next(step, () => undefined); this._tabsSwitched++; // console.log("nextTab", this._tabStepper.index); @@ -290,7 +310,7 @@ export class SearchModelImpl extends Disposable implements SearchModel { const section = this._sectionStepper.value; if (!section.viewInstance.peek()) { const view = this._tabStepper.value.view.peek(); - await this._gristDoc.openDocPage(view.getRowId()); + await this._openDocPage(view.getRowId()); console.log("SearchBar: loading view %s section %s", view.getRowId(), section.getRowId()); const viewInstance: any = await waitObs(section.viewInstance); await viewInstance.getLoadingDonePromise(); @@ -329,6 +349,12 @@ export class SearchModelImpl extends Disposable implements SearchModel { setTimeout(() => cursor.classList.remove('search-match'), 20); } } + + // Opens doc page without triggering a restart. + private async _openDocPage(viewId: number) { + await this._gristDoc.openDocPage(viewId); + this._isRestartNeeded = false; + } } function makeRegexp(value: string) { diff --git a/app/client/ui2018/search.ts b/app/client/ui2018/search.ts index dfcefb83..57e9a347 100644 --- a/app/client/ui2018/search.ts +++ b/app/client/ui2018/search.ts @@ -5,8 +5,10 @@ import { createGroup } from 'app/client/components/commands'; import { reportError } from 'app/client/models/AppModel'; import { SearchModel } from 'app/client/models/SearchModel'; +import { hoverTooltip, IHoverTipOptions } from 'app/client/ui/tooltips'; import { cssHoverCircle, cssTopBarBtn } from 'app/client/ui/TopBarCss'; -import { colors, mediaSmall } from 'app/client/ui2018/cssVars'; +import { labeledSquareCheckbox } from 'app/client/ui2018/checkbox'; +import { colors, mediaSmall, vars } from 'app/client/ui2018/cssVars'; import { icon } from 'app/client/ui2018/icons'; import { dom, input, styled } from 'grainjs'; import { noTestId, TestId } from 'grainjs'; @@ -27,6 +29,7 @@ const searchWrapper = styled('div', ` height: 100%; max-height: 50px; transition: width 0.4s; + position: relative; &-expand { width: 100% !important; border: 1px solid grey; @@ -71,13 +74,16 @@ const cssArrowBtn = styled('div', ` font-size: 14px; padding: 3px; cursor: pointer; - margin: 2px; + margin: 2px 4px; visibility: hidden; - - &.disabled { - color: ${colors.darkGrey}; - cursor: default; - } + width: 24px; + height: 24px; + background-color: ${colors.mediumGrey}; + --icon-color: ${colors.slate}; + border-radius: 3px; + text-align: center; + display: flex; + align-items: center; .${searchWrapper.className}-expand & { visibility: visible; @@ -86,9 +92,38 @@ const cssArrowBtn = styled('div', ` const cssCloseBtn = styled(icon, ` cursor: pointer; + background-color: ${colors.lightGreen}; + margin-left: 4px; + flex-shrink: 0; `); +const cssLabel = styled('span', ` + font-size: ${vars.smallFontSize}; + color: ${colors.slate}; + white-space: nowrap; + margin-right: 12px; +`); + +const cssOptions = styled('div', ` + position: absolute; + right: 0; + top: 48px; + z-index: 1; +`); + +const cssShortcut = styled('span', ` + color: ${colors.slate}; +`); + +const searchArrowBtnTooltipOptions: IHoverTipOptions = { + key: 'searchArrowBtnTooltip', + openDelay: 500, + closeDelay: 100, +}; + export function searchBar(model: SearchModel, testId: TestId = noTestId) { + let keepExpanded = false; + const commandGroup = createGroup({ find: () => { inputElem.focus(); inputElem.select(); }, // On Mac, Firefox has a default behaviour witch causes to close the search bar on Cmd+g and @@ -100,9 +135,13 @@ export function searchBar(model: SearchModel, testId: TestId = noTestId) { const toggleMenu = debounce((_value?: boolean) => { model.isOpen.set(_value === undefined ? !model.isOpen.get() : _value); }, 100); - const inputElem = searchInput(model.value, {onInput: true}, + const inputElem: HTMLInputElement = searchInput(model.value, {onInput: true}, {type: 'text', placeholder: 'Search in document'}, - dom.on('blur', () => toggleMenu(false)), + dom.on('blur', () => ( + keepExpanded ? + setTimeout(() => inputElem.focus(), 0) : + toggleMenu(false) + )), dom.onKeyDown({ Enter: () => model.findNext(), Escape: () => toggleMenu(false), @@ -131,21 +170,39 @@ export function searchBar(model: SearchModel, testId: TestId = noTestId) { expandedSearch( testId('input'), inputElem, - cssArrowBtn('\u2329', - testId('prev'), - // Prevent focus from being stolen from the input - dom.on('mousedown', (event) => event.preventDefault()), - dom.on('click', () => model.findPrev()), - dom.cls('disabled', model.noMatch)), - cssArrowBtn('\u232A', - testId('next'), - // Prevent focus from being stolen from the input - dom.on('mousedown', (event) => event.preventDefault()), - dom.on('click', () => model.findNext()), - dom.cls('disabled', model.noMatch)), + dom.domComputed((use) => { + const noMatch = use(model.noMatch); + const isEmpty = use(model.isEmpty); + if (isEmpty) { return null; } + if (noMatch) { return cssLabel("No results"); } + return [ + cssArrowBtn( + icon('Dropdown'), + testId('next'), + // Prevent focus from being stolen from the input + dom.on('mousedown', (event) => event.preventDefault()), + dom.on('click', () => model.findNext()), + hoverTooltip(() => ['Find Next ', cssShortcut('(Enter, ⌘G)')], searchArrowBtnTooltipOptions), + ), + cssArrowBtn( + icon('DropdownUp'), + testId('prev'), + // Prevent focus from being stolen from the input + dom.on('mousedown', (event) => event.preventDefault()), + dom.on('click', () => model.findPrev()), + hoverTooltip(() => ['Find Previous ', cssShortcut('(⌘⇧G)')], searchArrowBtnTooltipOptions), + ) + ]; + }), cssCloseBtn('CrossSmall', testId('close'), - dom.on('click', () => toggleMenu(false))) + dom.on('click', () => toggleMenu(false))), + cssOptions( + labeledSquareCheckbox(model.multiPage, 'Search all pages'), + dom.on('mouseenter', () => keepExpanded = true), + dom.on('mouseleave', () => keepExpanded = false), + testId('option-all-pages'), + ), ) ); }