From e033889b6abaedffdff61bcdb1933d9fa7cfed99 Mon Sep 17 00:00:00 2001 From: George Gevoian Date: Tue, 26 Sep 2023 20:40:34 -0400 Subject: [PATCH] (core) Fix calendar and card tip bug on mobile Summary: On mobile, tips for calendar and card list aren't currently shown, but the creator panel was still automatically being opened in preparation for showing the tip. Test Plan: Manual and existing tests. Reviewers: jarek Reviewed By: jarek Subscribers: jarek Differential Revision: https://phab.getgrist.com/D4053 --- .../components/BehavioralPromptsManager.ts | 92 ++++++++++--------- app/client/components/GristDoc.ts | 18 ++-- app/client/ui/CustomSectionConfig.ts | 6 +- app/client/ui/GristTooltips.ts | 40 +++++--- test/nbrowser/AttachedCustomWidget.ts | 2 +- 5 files changed, 88 insertions(+), 70 deletions(-) diff --git a/app/client/components/BehavioralPromptsManager.ts b/app/client/components/BehavioralPromptsManager.ts index 8627869d..2842e9ca 100644 --- a/app/client/components/BehavioralPromptsManager.ts +++ b/app/client/components/BehavioralPromptsManager.ts @@ -8,26 +8,32 @@ import {getGristConfig} from 'app/common/urlUtils'; import {Computed, Disposable, dom, Observable} from 'grainjs'; import {IPopupOptions} from 'popweasel'; -export interface AttachOptions { - /** Defaults to false. */ - forceShow?: boolean; - /** Defaults to false. */ +/** + * Options for showing a tip. + */ +export interface ShowTipOptions { + /** Defaults to `false`. */ hideArrow?: boolean; - /** Defaults to false. */ - hideDontShowTips?: boolean; - /** Defaults to true. */ - markAsSeen?: boolean; - /** Defaults to false. */ - showOnMobile?: boolean; popupOptions?: IPopupOptions; onDispose?(): void; - shouldShow?(): boolean; +} + +/** + * Options for attaching a tip to a DOM element. + */ +export interface AttachTipOptions extends ShowTipOptions { + /** + * Optional callback that should return true if the tip should be disabled. + * + * If omitted, the tip is enabled. + */ + isDisabled?(): boolean; } interface QueuedTip { prompt: BehavioralPrompt; refElement: Element; - options: AttachOptions; + options: ShowTipOptions; } /** @@ -52,12 +58,14 @@ export class BehavioralPromptsManager extends Disposable { super(); } - public showTip(refElement: Element, prompt: BehavioralPrompt, options: AttachOptions = {}) { + public showTip(refElement: Element, prompt: BehavioralPrompt, options: ShowTipOptions = {}) { this._queueTip(refElement, prompt, options); } - public attachTip(prompt: BehavioralPrompt, options: AttachOptions = {}) { + public attachTip(prompt: BehavioralPrompt, options: AttachTipOptions = {}) { return (element: Element) => { + if (options.isDisabled?.()) { return; } + this._queueTip(element, prompt, options); }; } @@ -70,6 +78,29 @@ export class BehavioralPromptsManager extends Disposable { return !this._prefs.get().dontShowTips; } + public shouldShowTip(prompt: BehavioralPrompt): boolean { + if (this._isDisabled) { return false; } + + const { + showContext = 'desktop', + showDeploymentTypes, + forceShow = false, + } = GristBehavioralPrompts[prompt]; + + const {deploymentType} = getGristConfig(); + if ( + showDeploymentTypes !== '*' && + (!deploymentType || !showDeploymentTypes.includes(deploymentType)) + ) { + return false; + } + + const context = isNarrowScreen() ? 'mobile' : 'desktop'; + if (showContext !== '*' && showContext !== context) { return false; } + + return forceShow || (!this._prefs.get().dontShowTips && !this.hasSeenTip(prompt)); + } + public enable() { this._isDisabled = false; } @@ -83,8 +114,8 @@ export class BehavioralPromptsManager extends Disposable { this.enable(); } - private _queueTip(refElement: Element, prompt: BehavioralPrompt, options: AttachOptions) { - if (!this._shouldQueueTip(prompt, options)) { return; } + private _queueTip(refElement: Element, prompt: BehavioralPrompt, options: ShowTipOptions) { + if (!this.shouldShowTip(prompt)) { return; } this._queuedTips.push({prompt, refElement, options}); if (this._queuedTips.length > 1) { @@ -96,15 +127,15 @@ export class BehavioralPromptsManager extends Disposable { this._showTip(refElement, prompt, options); } - private _showTip(refElement: Element, prompt: BehavioralPrompt, options: AttachOptions) { + private _showTip(refElement: Element, prompt: BehavioralPrompt, options: ShowTipOptions) { const close = () => { if (!ctl.isDisposed()) { ctl.close(); } }; - const {hideArrow, hideDontShowTips, markAsSeen = true, onDispose, popupOptions} = options; - const {title, content} = GristBehavioralPrompts[prompt]; + const {hideArrow, onDispose, popupOptions} = options; + const {title, content, hideDontShowTips = false, markAsSeen = true} = GristBehavioralPrompts[prompt]; const ctl = showBehavioralPrompt(refElement, title(), content(), { onClose: (dontShowTips) => { if (dontShowTips) { this._dontShowTips(); } @@ -143,27 +174,4 @@ export class BehavioralPromptsManager extends Disposable { this._prefs.set({...this._prefs.get(), dontShowTips: true}); this._queuedTips = []; } - - private _shouldQueueTip(prompt: BehavioralPrompt, options: AttachOptions) { - if ( - this._isDisabled || - options.shouldShow?.() === false || - (isNarrowScreen() && !options.showOnMobile) || - (this._prefs.get().dontShowTips && !options.forceShow) || - this.hasSeenTip(prompt) - ) { - return false; - } - - const {deploymentType} = getGristConfig(); - const {deploymentTypes} = GristBehavioralPrompts[prompt]; - if ( - deploymentTypes !== '*' && - (!deploymentType || !deploymentTypes.includes(deploymentType)) - ) { - return false; - } - - return true; - } } diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index fc93cabf..84ba6dee 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -336,10 +336,6 @@ export class GristDoc extends DisposableWithEvents { } this.behavioralPromptsManager.showTip(cursor, 'rickRow', { - forceShow: true, - hideDontShowTips: true, - markAsSeen: false, - showOnMobile: true, onDispose: () => this.playRickRollVideo(), }); }) @@ -1422,8 +1418,8 @@ export class GristDoc extends DisposableWithEvents { if ( // Don't show the tip if a non-card widget was selected. !['single', 'detail'].includes(selectedWidgetType) || - // Or if we've already seen it. - this.behavioralPromptsManager.hasSeenTip('editCardLayout') + // Or if we shouldn't see the tip. + !this.behavioralPromptsManager.shouldShowTip('editCardLayout') ) { return; } @@ -1449,11 +1445,13 @@ export class GristDoc extends DisposableWithEvents { private async _handleNewAttachedCustomWidget(widget: IAttachedCustomWidget) { switch (widget) { case 'custom.calendar': { - // Open the right panel to the calendar subtab. - commands.allCommands.viewTabOpen.run(); + if (this.behavioralPromptsManager.shouldShowTip('calendarConfig')) { + // Open the right panel to the calendar subtab. + commands.allCommands.viewTabOpen.run(); - // Wait for the right panel to finish animation if it was collapsed before. - await commands.allCommands.rightPanelOpen.run(); + // Wait for the right panel to finish animation if it was collapsed before. + await commands.allCommands.rightPanelOpen.run(); + } break; } } diff --git a/app/client/ui/CustomSectionConfig.ts b/app/client/ui/CustomSectionConfig.ts index 234e1743..8efff470 100644 --- a/app/client/ui/CustomSectionConfig.ts +++ b/app/client/ui/CustomSectionConfig.ts @@ -481,9 +481,9 @@ export class CustomSectionConfig extends Disposable { popupOptions: { placement: 'left-start', }, - shouldShow: () => { - // Only show tip if a custom widget isn't already selected. - return !this._selectedId.get() || (isCustom.get() && this._url.get().trim() === ''); + isDisabled: () => { + // Disable tip if a custom widget is already selected. + return Boolean(this._selectedId.get() && !(isCustom.get() && this._url.get().trim() === '')); }, }) ), diff --git a/app/client/ui/GristTooltips.ts b/app/client/ui/GristTooltips.ts index ddfff8ef..27ad4bf0 100644 --- a/app/client/ui/GristTooltips.ts +++ b/app/client/ui/GristTooltips.ts @@ -103,7 +103,15 @@ see or edit which parts of your document.') export interface BehavioralPromptContent { title: () => string; content: (...domArgs: DomElementArg[]) => DomContents; - deploymentTypes: GristDeploymentType[] | '*'; + showDeploymentTypes: GristDeploymentType[] | '*'; + /** Defaults to `desktop`. */ + showContext?: 'mobile' | 'desktop' | '*'; + /** Defaults to `false`. */ + hideDontShowTips?: boolean; + /** Defaults to `false`. */ + forceShow?: boolean; + /** Defaults to `true`. */ + markAsSeen?: boolean; } export const GristBehavioralPrompts: Record = { @@ -119,7 +127,7 @@ export const GristBehavioralPrompts: Record t('Reference Columns'), @@ -134,7 +142,7 @@ record in that table, but you may select which column from that record to show.' ), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, rawDataPage: { title: () => t('Raw Data page'), @@ -144,7 +152,7 @@ including summary tables and tables not included in page layouts.')), dom('div', cssLink({href: commonUrls.helpRawData, target: '_blank'}, t('Learn more.'))), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, accessRules: { title: () => t('Access Rules'), @@ -154,7 +162,7 @@ to determine who can see or edit which parts of your document.')), dom('div', cssLink({href: commonUrls.helpAccessRules, target: '_blank'}, t('Learn more.'))), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, filterButtons: { title: () => t('Pinning Filters'), @@ -164,7 +172,7 @@ to determine who can see or edit which parts of your document.')), dom('div', cssLink({href: commonUrls.helpFilterButtons, target: '_blank'}, t('Learn more.'))), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, nestedFiltering: { title: () => t('Nested Filtering'), @@ -173,7 +181,7 @@ to determine who can see or edit which parts of your document.')), dom('div', t('Only those rows will appear which match all of the filters.')), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, pageWidgetPicker: { title: () => t('Selecting Data'), @@ -182,7 +190,7 @@ to determine who can see or edit which parts of your document.')), dom('div', t('Use the 𝚺 icon to create summary (or pivot) tables, for totals or subtotals.')), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, pageWidgetPickerSelectBy: { title: () => t('Linking Widgets'), @@ -192,7 +200,7 @@ to determine who can see or edit which parts of your document.')), dom('div', cssLink({href: commonUrls.helpLinkingWidgets, target: '_blank'}, t('Learn more.'))), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, editCardLayout: { title: () => t('Editing Card Layout'), @@ -203,7 +211,7 @@ to determine who can see or edit which parts of your document.')), })), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, addNew: { title: () => t('Add New'), @@ -211,7 +219,7 @@ to determine who can see or edit which parts of your document.')), dom('div', t('Click the Add New button to create new documents or workspaces, or import data.')), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, rickRow: { title: () => t('Anchor Links'), @@ -225,7 +233,11 @@ to determine who can see or edit which parts of your document.')), ), ...args, ), - deploymentTypes: '*', + showDeploymentTypes: '*', + showContext: '*', + hideDontShowTips: true, + forceShow: true, + markAsSeen: false, }, customURL: { title: () => t('Custom Widgets'), @@ -238,7 +250,7 @@ to determine who can see or edit which parts of your document.')), dom('div', cssLink({href: commonUrls.helpCustomWidgets, target: '_blank'}, t('Learn more.'))), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, calendarConfig: { title: () => t('Calendar'), @@ -250,6 +262,6 @@ data.")), dom('div', cssLink({href: commonUrls.helpCalendarWidget, target: '_blank'}, t('Learn more.'))), ...args, ), - deploymentTypes: ['saas'], + showDeploymentTypes: ['saas'], }, }; diff --git a/test/nbrowser/AttachedCustomWidget.ts b/test/nbrowser/AttachedCustomWidget.ts index 2be96d50..24256b71 100644 --- a/test/nbrowser/AttachedCustomWidget.ts +++ b/test/nbrowser/AttachedCustomWidget.ts @@ -84,7 +84,7 @@ describe('AttachedCustomWidget', function () { it('should not ask for permission', async () => { await gu.addNewSection(/Calendar/, /Table1/, {selectBy: /TABLE1/}); await gu.getSection('TABLE1 Calendar').click(); - await gu.waitForSidePanel(); + await gu.toggleSidePanel('right', 'open'); await driver.find('.test-right-tab-pagewidget').click(); await gu.waitForServer();