From 3b76b33423c48b2d7102f1c9d68594d416a94b30 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Sun, 20 Mar 2022 23:41:59 -0400 Subject: [PATCH] (core) Fix bugs when both welcomeTour and docTour are available Summary: - Unify where in the code tours get initiated. - Avoid start a new tour while one is being started or is in progress. - Ignore welcome tour when on a doc that has a doc tour. - Fix tours when starting with a special page like Access Rules. - Remove mention of the no-longer-present "Give Feedback" button in the last message of the welcome tour. Test Plan: Add a browser test case that docTour preempts the welcome tour and shows no errors (this test case fails in multiple ways without the changes). Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3330 --- app/client/components/GristDoc.ts | 65 +++++++++++++++++++++++-------- app/client/models/UserPrefs.ts | 20 ++++++++++ app/client/ui/OnBoardingPopups.ts | 5 +++ app/client/ui/Tools.ts | 38 +++--------------- app/client/ui/welcomeTour.ts | 3 +- 5 files changed, 79 insertions(+), 52 deletions(-) diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index b3b09a4c..c791ce5a 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -35,11 +35,12 @@ import {DocPageModel} from 'app/client/models/DocPageModel'; import {UserError} from 'app/client/models/errors'; import {urlState} from 'app/client/models/gristUrlState'; import {getFilterFunc, QuerySetManager} from 'app/client/models/QuerySet'; -import {getUserOrgPrefObs, getUserOrgPrefsObs} from 'app/client/models/UserPrefs'; +import {getUserOrgPrefObs, getUserOrgPrefsObs, markAsSeen} from 'app/client/models/UserPrefs'; import {App} from 'app/client/ui/App'; import {DocHistory} from 'app/client/ui/DocHistory'; import {startDocTour} from "app/client/ui/DocTour"; import {showDocSettingsModal} from 'app/client/ui/DocumentSettings'; +import {isTourActive} from "app/client/ui/OnBoardingPopups"; import {IPageWidget, toPageWidget} from 'app/client/ui/PageWidgetPicker'; import {IPageWidgetLink, linkFromId, selectBy} from 'app/client/ui/selectBy'; import {startWelcomeTour} from 'app/client/ui/welcomeTour'; @@ -132,6 +133,9 @@ export class GristDoc extends DisposableWithEvents { public readonly userOrgPrefs = getUserOrgPrefsObs(this.docPageModel.appModel); + // If the doc has a docTour. Used also to enable the UI button to restart the tour. + public readonly hasDocTour: Computed; + private _actionLog: ActionLog; private _undoStack: UndoStack; private _lastOwnActionGroup: ActionGroupWithCursorPos|null = null; @@ -140,6 +144,7 @@ export class GristDoc extends DisposableWithEvents { private _rightPanelTool = createSessionObs(this, "rightPanelTool", "none", RightPanelTool.guard); private _viewLayout: ViewLayout|null = null; private _showGristTour = getUserOrgPrefObs(this.userOrgPrefs, 'showGristTour'); + private _seenDocTours = getUserOrgPrefObs(this.userOrgPrefs, 'seenDocTours'); constructor( public readonly app: App, @@ -162,6 +167,9 @@ export class GristDoc extends DisposableWithEvents { // Maintain the MetaRowModel for the global document info, including docId and peers. this.docInfo = this.docModel.docInfoRow; + this.hasDocTour = Computed.create(this, use => + use(this.docModel.allTableIds.getObservable()).includes('GristDocTour')); + const defaultViewId = this.docInfo.newDefaultViewId; // Grainjs observable for current view id, which may be a string such as 'code'. @@ -217,24 +225,39 @@ export class GristDoc extends DisposableWithEvents { })); // Start welcome tour if flag is present in the url hash. + let tourStarting = false; this.autoDispose(subscribe(urlState().state, async (_use, state) => { - if (state.welcomeTour || state.docTour || this._shouldAutoStartWelcomeTour()) { - // On boarding tours were not designed with mobile support in mind. Disable until fixed. - if (isNarrowScreen()) { - return; - } - await this._waitForView(); - await delay(0); // we need to wait an extra bit. + // Onboarding tours were not designed with mobile support in mind. Disable until fixed. + if (isNarrowScreen()) { + return; + } + // If we have an active tour (or are in the process of starting one), don't start a new one. + if (tourStarting || isTourActive()) { + return; + } + const autoStartDocTour = this.hasDocTour.get() && !this._seenDocTours.get()?.includes(this.docId()); + const docTour = state.docTour || autoStartDocTour; + const welcomeTour = state.welcomeTour || this._shouldAutoStartWelcomeTour(); - // Remove any tour-related hash-tags from the URL. So #repeat-welcome-tour and - // #repeat-doc-tour are used as triggers, but will immediately disappear. - await urlState().pushUrl({welcomeTour: false, docTour: false}, - {replace: true, avoidReload: true}); + if (welcomeTour || docTour) { + tourStarting = true; + try { + await this._waitForView(); + await delay(0); // we need to wait an extra bit. - if (!state.docTour) { - startWelcomeTour(() => this._showGristTour.set(false)); - } else { - await startDocTour(this.docData, this.docComm, () => null); + // Remove any tour-related hash-tags from the URL. So #repeat-welcome-tour and + // #repeat-doc-tour are used as triggers, but will immediately disappear. + await urlState().pushUrl({welcomeTour: false, docTour: false}, + {replace: true, avoidReload: true}); + + if (!docTour) { + startWelcomeTour(() => this._showGristTour.set(false)); + } else { + const onFinishCB = () => (autoStartDocTour && markAsSeen(this._seenDocTours, this.docId())); + await startDocTour(this.docData, this.docComm, onFinishCB); + } + } finally { + tourStarting = false; } } })); @@ -801,6 +824,10 @@ export class GristDoc extends DisposableWithEvents { * Waits for a view to be ready */ private async _waitForView() { + // For pages like ACL's, there isn't a view instance to wait for. + if (!this.viewModel.activeSection.peek().getRowId()) { + return; + } const view = await waitObs(this.viewModel.activeSection.peek().viewInstance); await view?.getLoadingDonePromise(); return view; @@ -929,7 +956,11 @@ export class GristDoc extends DisposableWithEvents { * For first-time users on personal org, start a welcome tour. */ private _shouldAutoStartWelcomeTour(): boolean { - // TODO: decide what to do when both a docTour and grist welcome tour are available. + // When both a docTour and grist welcome tour are available, show only the docTour, leaving + // the welcome tour for another doc (e.g. a new one). + if (this.hasDocTour.get()) { + return false; + } // Only show the tour if one is on a personal org and can edit. This excludes templates (on // the Templates org, which may have their own tour) and team sites (where user's intended diff --git a/app/client/models/UserPrefs.ts b/app/client/models/UserPrefs.ts index 625152c8..543879bd 100644 --- a/app/client/models/UserPrefs.ts +++ b/app/client/models/UserPrefs.ts @@ -58,3 +58,23 @@ function makePrefFunctions

(prefsTypeName: P) { export const {getPrefsObs: getUserOrgPrefsObs, getPrefObs: getUserOrgPrefObs} = makePrefFunctions('userOrgPrefs'); export const {getPrefsObs: getUserPrefsObs, getPrefObs: getUserPrefObs} = makePrefFunctions('userPrefs'); + + +// For preferences that store a list of items (such as seen docTours), this helper updates the +// preference to add itemId to it (e.g. to avoid auto-starting the docTour again in the future). +// prefKey is used only to log a more informative warning on error. +export function markAsSeen(seenIdsObs: Observable, itemId: T) { + const seenIds = seenIdsObs.get() || []; + try { + if (!seenIds.includes(itemId)) { + const seen = new Set(seenIds); + seen.add(itemId); + seenIdsObs.set([...seen].sort()); + } + } catch (e) { + // If we fail to save this preference, it's probably not worth alerting the user about, + // so just log to console. + // tslint:disable-next-line:no-console + console.warn("Failed to save preference in markAsSeen", e); + } +} diff --git a/app/client/ui/OnBoardingPopups.ts b/app/client/ui/OnBoardingPopups.ts index b06bd229..692d92c3 100644 --- a/app/client/ui/OnBoardingPopups.ts +++ b/app/client/ui/OnBoardingPopups.ts @@ -80,6 +80,11 @@ export function startOnBoarding(messages: IOnBoardingMsg[], onFinishCB: () => vo ctl.start().catch(reportError); } +// Returns whether some tour is currently active. +export function isTourActive(): boolean { + return !tourSingleton.isEmpty(); +} + class OnBoardingError extends Error { public name = 'OnBoardingError'; constructor(message: string) { diff --git a/app/client/ui/Tools.ts b/app/client/ui/Tools.ts index 0843f60c..5b3d49e9 100644 --- a/app/client/ui/Tools.ts +++ b/app/client/ui/Tools.ts @@ -1,7 +1,6 @@ import {GristDoc} from 'app/client/components/GristDoc'; -import {loadGristDoc} from 'app/client/lib/imports'; import {urlState} from 'app/client/models/gristUrlState'; -import {getUserOrgPrefObs} from 'app/client/models/UserPrefs'; +import {getUserOrgPrefObs, markAsSeen} from 'app/client/models/UserPrefs'; import {showExampleCard} from 'app/client/ui/ExampleCard'; import {examples} from 'app/client/ui/ExampleInfo'; import {createHelpTools, cssLinkText, cssPageEntry, cssPageEntryMain, cssPageEntrySmall, @@ -14,7 +13,7 @@ import {cssLink} from 'app/client/ui2018/links'; import {menuAnnotate} from 'app/client/ui2018/menus'; import {confirmModal} from 'app/client/ui2018/modals'; import {userOverrideParams} from 'app/common/gristUrls'; -import {Computed, Disposable, dom, makeTestId, Observable, observable, styled} from 'grainjs'; +import {Disposable, dom, makeTestId, Observable, observable, styled} from 'grainjs'; const testId = makeTestId('test-tools-'); @@ -22,8 +21,6 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse const docPageModel = gristDoc.docPageModel; const isOwner = docPageModel.currentDoc.get()?.access === 'owners'; const isOverridden = Boolean(docPageModel.userOverride.get()); - const hasDocTour = Computed.create(owner, use => - use(gristDoc.docModel.allTableIds.getObservable()).includes('GristDocTour')); const canViewAccessRules = observable(false); function updateCanViewAccessRules() { canViewAccessRules.set((isOwner && !isOverridden) || @@ -106,20 +103,12 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse ); }), // Show the 'Tour of this Document' button if a GristDocTour table exists. - dom.maybe(hasDocTour, () => + dom.maybe(gristDoc.hasDocTour, () => cssSplitPageEntry( cssPageEntryMain( cssPageLink(cssPageIcon('Page'), cssLinkText('Tour of this Document'), - automaticHelpTool( - async ({markAsSeen}) => { - const gristDocModule = await loadGristDoc(); - await gristDocModule.startDocTour(gristDoc.docData, gristDoc.docComm, markAsSeen); - }, - gristDoc, - "seenDocTours", - gristDoc.docId() - ), + urlState().setLinkUrl({docTour: true}), testId('doctour'), ), ), @@ -165,24 +154,7 @@ function automaticHelpTool( return; } - // When the help is closed, if it's the first time it's dismissed, save this fact, to avoid - // showing it automatically again in the future. - function markAsSeen() { - try { - if (!seenIds.includes(itemId)) { - const seen = new Set(seenIds); - seen.add(itemId); - prefObs.set([...seen].sort()); - } - } catch (e) { - // If we fail to save this preference, it's probably not worth alerting the user about, - // so just log to console. - // tslint:disable-next-line:no-console - console.warn("Failed to save userPref " + prefKey, e); - } - } - - showFunc({elem, reopen, markAsSeen}); + showFunc({elem, reopen, markAsSeen: () => markAsSeen(prefObs, itemId)}); } return [ diff --git a/app/client/ui/welcomeTour.ts b/app/client/ui/welcomeTour.ts index 674fc22b..fb0deae6 100644 --- a/app/client/ui/welcomeTour.ts +++ b/app/client/ui/welcomeTour.ts @@ -63,8 +63,7 @@ export const welcomeTour: IOnBoardingMsg[] = [ selector: '.tour-help-center', title: 'Flying higher', body: () => [ - dom('p', 'Use ', Key(GreyIcon('Help'), 'Help Center'), ' for documentation, videos, and tutorials.'), - dom('p', 'Use ', Key(GreyIcon('Feedback'), 'Give Feedback'), ' for issues or questions.'), + dom('p', 'Use ', Key(GreyIcon('Help'), 'Help Center'), ' for documentation or questions.'), ], placement: 'right', },