From cd0bec5d318a6c58f614b3c664ba7d10b512a6ba Mon Sep 17 00:00:00 2001 From: pengx17 Date: Fri, 28 Feb 2025 02:34:28 +0000 Subject: [PATCH] fix(core): at menu ux (#10485) fix AF-2285 1. loading icon will be rendered to the group name 2. make the focused item more stable --- .../src/widgets/linked-doc/config.ts | 7 +- .../widgets/linked-doc/linked-doc-popover.ts | 127 +++++++++++------- .../src/widgets/linked-doc/styles.ts | 12 ++ .../modules/at-menu-config/services/index.ts | 37 +++-- .../modules/doc-search-menu/services/index.ts | 6 +- tests/affine-local/e2e/links.spec.ts | 2 +- 6 files changed, 120 insertions(+), 71 deletions(-) diff --git a/blocksuite/affine/block-root/src/widgets/linked-doc/config.ts b/blocksuite/affine/block-root/src/widgets/linked-doc/config.ts index 0b27db500e..aa8faa399e 100644 --- a/blocksuite/affine/block-root/src/widgets/linked-doc/config.ts +++ b/blocksuite/affine/block-root/src/widgets/linked-doc/config.ts @@ -55,12 +55,13 @@ export interface LinkedWidgetConfig { * * If the return value is not null, no action will be taken. */ - autoFocusedItem?: ( + autoFocusedItemKey?: ( menus: LinkedMenuGroup[], query: string, + currentActiveKey: string | null, editorHost: EditorHost, inlineEditor: AffineInlineEditor - ) => LinkedMenuItem | null; + ) => string | null; mobile: { useScreenHeight?: boolean; @@ -101,8 +102,6 @@ export type LinkedMenuGroup = { loading?: boolean | Signal; // copywriting when display quantity exceeds overflowText?: string | Signal; - // loading text - loadingText?: string | Signal; }; export type LinkedDocContext = { diff --git a/blocksuite/affine/block-root/src/widgets/linked-doc/linked-doc-popover.ts b/blocksuite/affine/block-root/src/widgets/linked-doc/linked-doc-popover.ts index 088fd2c3a7..62b32a454e 100644 --- a/blocksuite/affine/block-root/src/widgets/linked-doc/linked-doc-popover.ts +++ b/blocksuite/affine/block-root/src/widgets/linked-doc/linked-doc-popover.ts @@ -75,23 +75,33 @@ export class LinkedDocPopover extends SignalWatcher( // need to rebind the effect because this._linkedDocGroup has changed. this._menusItemsEffectCleanup = effect(() => { this._updateAutoFocusedItem(); + + // wait for the next tick to ensure the items are rendered to DOM + setTimeout(() => { + this.scrollToFocusedItem(); + }); }); }; private readonly _updateAutoFocusedItem = () => { - if (!this._query) { - return; - } - const autoFocusedItem = this.context.config.autoFocusedItem?.( + // Get the auto-focused item key from the config + const autoFocusedItemKey = this.context.config.autoFocusedItemKey?.( this._linkedDocGroup, - this._query, + this._query || '', + this._activatedItemKey, this.context.std.host, this.context.inlineEditor ); - if (autoFocusedItem) { - this._activatedItemIndex = this._flattenActionList.findIndex( - item => item.key === autoFocusedItem.key - ); + + if (autoFocusedItemKey) { + this._activatedItemKey = autoFocusedItemKey; + return; + } + + // If no auto-focused item key is returned from the config and no item is currently focused, + // focus the first item in the flattened action list + if (!this._activatedItemKey && this._flattenActionList.length > 0) { + this._activatedItemKey = this._flattenActionList[0].key; } }; @@ -126,19 +136,9 @@ export class LinkedDocPopover extends SignalWatcher( let items = resolveSignal(group.items); const isOverflow = !!group.maxDisplay && items.length > group.maxDisplay; - const isLoading = resolveSignal(group.loading); items = isExpanded ? items : items.slice(0, group.maxDisplay); - if (isLoading) { - items = items.concat({ - key: 'loading', - name: resolveSignal(group.loadingText) || 'loading', - icon: LoadingIcon, - action: () => {}, - }); - } - if (isOverflow && !isExpanded && group.maxDisplay) { items = items.concat({ key: `${group.name} More`, @@ -183,7 +183,6 @@ export class LinkedDocPopover extends SignalWatcher( target: eventSource, signal: keydownObserverAbortController.signal, onInput: isComposition => { - this._activatedItemIndex = 0; if (isComposition) { this._updateLinkedDocGroup().catch(console.error); } else { @@ -193,7 +192,6 @@ export class LinkedDocPopover extends SignalWatcher( } }, onPaste: () => { - this._activatedItemIndex = 0; setTimeout(() => { this._updateLinkedDocGroup().catch(console.error); }, 50); @@ -206,33 +204,18 @@ export class LinkedDocPopover extends SignalWatcher( if (curRange.index < this.context.startRange.index) { this.context.close(); } - this._activatedItemIndex = 0; this.context.inlineEditor.slots.renderComplete.once( this._updateLinkedDocGroup ); }, onMove: step => { const itemLen = this._flattenActionList.length; - this._activatedItemIndex = - (itemLen + this._activatedItemIndex + step) % itemLen; - - // Scroll to the active item - const item = this._flattenActionList[this._activatedItemIndex]; - const shadowRoot = this.shadowRoot; - if (!shadowRoot) { - console.warn('Failed to find the shadow root!', this); - return; + const nextIndex = (itemLen + this._activatedItemIndex + step) % itemLen; + const item = this._flattenActionList[nextIndex]; + if (item) { + this._activatedItemKey = item.key; } - const ele = shadowRoot.querySelector( - `icon-button[data-id="${item.key}"]` - ); - if (!ele) { - console.warn('Failed to find the active item!', item); - return; - } - ele.scrollIntoView({ - block: 'nearest', - }); + this.scrollToFocusedItem(); }, onConfirm: () => { this._flattenActionList[this._activatedItemIndex] @@ -261,19 +244,29 @@ export class LinkedDocPopover extends SignalWatcher( visibility: 'hidden', }); - // XXX This is a side effect - let accIdx = 0; + const actionGroups = this._actionGroup.map(group => { + // Check if the group is loading + const isLoading = resolveSignal(group.loading); + return { + ...group, + isLoading, + }; + }); + return html`
- ${this._actionGroup - .filter(group => group.items.length) + ${actionGroups + .filter(group => group.items.length || group.isLoading) .map((group, idx) => { return html`
-
${group.name}
+
+ ${group.name} + ${group.isLoading + ? html`${LoadingIcon}` + : nothing} +
${group.items.map(({ key, name, icon, action }) => { - accIdx++; - const curIdx = accIdx - 1; const tooltip = this._showTooltip ? html` { action()?.catch(console.error); }} @mousemove=${() => { // Use `mousemove` instead of `mouseover` to avoid navigate conflict with keyboard - this._activatedItemIndex = curIdx; + this._activatedItemKey = key; // show tooltip whether text length overflows for (const button of this.iconButtons.values()) { if (button.dataset.id == key && button.textElement) { @@ -348,8 +341,40 @@ export class LinkedDocPopover extends SignalWatcher( } } + private scrollToFocusedItem() { + const shadowRoot = this.shadowRoot; + if (!shadowRoot) { + return; + } + + // If there's no active item key, don't try to scroll + if (!this._activatedItemKey) { + return; + } + + const ele = shadowRoot.querySelector( + `icon-button[data-id="${this._activatedItemKey}"]` + ); + + // If the element doesn't exist, don't log a warning + if (!ele) { + return; + } + + ele.scrollIntoView({ + block: 'nearest', + }); + } + + get _activatedItemIndex() { + const index = this._flattenActionList.findIndex( + item => item.key === this._activatedItemKey + ); + return index === -1 ? 0 : index; + } + @state() - private accessor _activatedItemIndex = 0; + private accessor _activatedItemKey: string | null = null; @state() private accessor _linkedDocGroup: LinkedMenuGroup[] = []; diff --git a/blocksuite/affine/block-root/src/widgets/linked-doc/styles.ts b/blocksuite/affine/block-root/src/widgets/linked-doc/styles.ts index ee3adb3f36..5bb150ebbf 100644 --- a/blocksuite/affine/block-root/src/widgets/linked-doc/styles.ts +++ b/blocksuite/affine/block-root/src/widgets/linked-doc/styles.ts @@ -51,6 +51,18 @@ export const linkedDocPopoverStyles = css` align-items: center; flex-shrink: 0; font-weight: 500; + justify-content: space-between; + } + + .linked-doc-popover .group-title .loading-icon { + display: flex; + align-items: center; + margin-left: 8px; + } + + .linked-doc-popover .group-title .loading-icon svg { + width: 20px; + height: 20px; } .linked-doc-popover .divider { diff --git a/packages/frontend/core/src/modules/at-menu-config/services/index.ts b/packages/frontend/core/src/modules/at-menu-config/services/index.ts index cc8e4086b4..88d0583ca7 100644 --- a/packages/frontend/core/src/modules/at-menu-config/services/index.ts +++ b/packages/frontend/core/src/modules/at-menu-config/services/index.ts @@ -32,6 +32,12 @@ function resolveSignal(data: T | Signal): T { return data instanceof Signal ? data.value : data; } +const RESERVED_ITEM_KEYS = { + createPage: 'create:page', + createEdgeless: 'create:edgeless', + datePicker: 'date-picker', +}; + export class AtMenuConfigService extends Service { constructor( private readonly journalService: JournalService, @@ -50,7 +56,7 @@ export class AtMenuConfigService extends Service { return { getMenus: this.getMenusFn(), mobile: this.getMobileConfig(), - autoFocusedItem: this.autoFocusedItem, + autoFocusedItemKey: this.autoFocusedItemKey, }; } @@ -61,18 +67,27 @@ export class AtMenuConfigService extends Service { }); } - private readonly autoFocusedItem = ( + private readonly autoFocusedItemKey = ( menus: LinkedMenuGroup[], - query: string - ): LinkedMenuItem | null => { + query: string, + currentActiveKey: string | null + ): string | null => { if (query.trim().length === 0) { return null; } + + if ( + currentActiveKey === RESERVED_ITEM_KEYS.createPage || + currentActiveKey === RESERVED_ITEM_KEYS.createEdgeless + ) { + return currentActiveKey; + } + // if the second group (linkToDocGroup) is EMPTY, // if the query is NOT empty && the second group (linkToDocGroup) is EMPTY, // we will focus on the first item of the third group (create), which is the "New Doc" item. if (resolveSignal(menus[1].items).length === 0) { - return resolveSignal(menus[2].items)[0]; + return resolveSignal(menus[2].items)[0]?.key; } return null; }; @@ -95,11 +110,9 @@ export class AtMenuConfigService extends Service { ? originalNewDocMenuGroup.items : originalNewDocMenuGroup.items.value; - const newDocItem = items.find(item => item.key === 'create'); const importItem = items.find(item => item.key === 'import'); - // should have both new doc and import item - if (!newDocItem || !importItem) { + if (!importItem) { return originalNewDocMenuGroup; } @@ -117,7 +130,7 @@ export class AtMenuConfigService extends Service { const customNewDocItems: LinkedMenuItem[] = [ { - key: 'create-page', + key: RESERVED_ITEM_KEYS.createPage, icon: NewXxxPageIcon(), name: I18n.t('com.affine.editor.at-menu.create-page', { name: query || I18n.t('Untitled'), @@ -132,7 +145,7 @@ export class AtMenuConfigService extends Service { }, }, { - key: 'create-edgeless', + key: RESERVED_ITEM_KEYS.createEdgeless, icon: NewXxxEdgelessIcon(), name: I18n.t('com.affine.editor.at-menu.create-edgeless', { name: query || I18n.t('Untitled'), @@ -190,7 +203,7 @@ export class AtMenuConfigService extends Service { const items: LinkedMenuItem[] = [ { icon: DateTimeIcon(), - key: 'date-picker', + key: RESERVED_ITEM_KEYS.datePicker, name: I18n.t('com.affine.editor.at-menu.date-picker'), action: () => { close(); @@ -245,7 +258,7 @@ export class AtMenuConfigService extends Service { items.unshift({ icon: icon(), - key: dateString, + key: RESERVED_ITEM_KEYS.datePicker + ':' + dateString, name: alias ? html`${alias}, status.remaining !== undefined && status.remaining > 0) + map(status => status.remaining !== undefined && status.remaining > 0), + takeWhile(isLoading => isLoading, true) ), false ); @@ -127,7 +128,6 @@ export class DocSearchMenuService extends Service { query, }), loading: isIndexerLoading, - loadingText: I18n.t('com.affine.editor.at-menu.loading'), items: docsSignal, maxDisplay: MAX_DOCS, overflowText, diff --git a/tests/affine-local/e2e/links.spec.ts b/tests/affine-local/e2e/links.spec.ts index 15d67c0c8b..ea7d54eb4b 100644 --- a/tests/affine-local/e2e/links.spec.ts +++ b/tests/affine-local/e2e/links.spec.ts @@ -483,7 +483,7 @@ test('@ popover can auto focus on the "New Doc" item when query returns no items await expect(page.locator('.linked-doc-popover')).toBeVisible(); const newDocMenuItem = page .locator('.linked-doc-popover') - .locator('[data-id="create-page"]'); + .locator('[data-id="create:page"]'); await expect(newDocMenuItem).toBeVisible(); await expect(newDocMenuItem).toHaveAttribute('hover', 'true'); });