From d1ed958336a3f04371843583961e6aa512c2985d Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Thu, 10 Mar 2022 10:21:28 +0000 Subject: [PATCH 1/3] chore(GlFilteredSearch): Name story components This aids debugging via Vue Devtools. --- src/components/base/filtered_search/filtered_search.stories.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/base/filtered_search/filtered_search.stories.js b/src/components/base/filtered_search/filtered_search.stories.js index a21c045b9b..b5532882fa 100644 --- a/src/components/base/filtered_search/filtered_search.stories.js +++ b/src/components/base/filtered_search/filtered_search.stories.js @@ -36,6 +36,7 @@ const fakeLabels = [ ]; const UserToken = { + name: 'UserToken', components: { GlFilteredSearchToken, GlFilteredSearchSuggestion, GlLoadingIcon, GlAvatar }, props: ['value', 'active'], inheritAttrs: false, @@ -112,6 +113,7 @@ const UserToken = { }; const MilestoneToken = { + name: 'MilestoneToken', components: { GlFilteredSearchToken, GlFilteredSearchSuggestion, GlLoadingIcon }, props: ['value', 'active'], inheritAttrs: false, @@ -172,6 +174,7 @@ const MilestoneToken = { }; const LabelToken = { + name: 'LabelToken', components: { GlFilteredSearchToken, GlFilteredSearchSuggestion, GlLoadingIcon, GlToken }, props: ['value', 'active'], inheritAttrs: false, -- GitLab From ead7f216ad1e18190382b7b692fc94414c945117 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Thu, 10 Mar 2022 10:23:28 +0000 Subject: [PATCH 2/3] refactor(GlFilteredSearch): Rename parameter It's an index, not a token. --- src/components/base/filtered_search/filtered_search.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/base/filtered_search/filtered_search.vue b/src/components/base/filtered_search/filtered_search.vue index b1b63103a0..ae379b1611 100644 --- a/src/components/base/filtered_search/filtered_search.vue +++ b/src/components/base/filtered_search/filtered_search.vue @@ -173,8 +173,8 @@ export default { return this.getTokenEntry(type)?.token || GlFilteredSearchTerm; }, - activate(token) { - this.activeTokenIdx = token; + activate(idx) { + this.activeTokenIdx = idx; }, alignSuggestions(ref) { -- GitLab From 40b051307065ff29fea53ad65457d6b99f116c92 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Thu, 10 Mar 2022 10:26:21 +0000 Subject: [PATCH 3/3] fix(GlFilteredSearch): Token activation after destruction This makes more intuitive the behaviour of the active token after a token has been destroyed. The previous behaviour was to always activate the previous token, unless the token being deleted is the first one. This had two undesirable effects: 1. If you click the close `x` button on a token, the previous token's value would now become active. This likely isn't what you want, and is what https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1679 is about. 2. If an active token is not the first token, and you click the close `x` on the first token, the currently active token changes, because all tokens got shifted down by one index. The new logic tries to maintain the active states of the tokens as-is, unless it's not possible, or the way the token was destroyed implies the previous token should be activated. That happens when a search term is destroyed by pressing backspace once it's empty. Addresses https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1679. --- .../filtered_search/filtered_search.spec.js | 118 ++++++++++++++++-- .../base/filtered_search/filtered_search.vue | 23 +++- .../filtered_search_term.spec.js | 23 ++-- .../filtered_search/filtered_search_term.vue | 8 +- .../filtered_search/filtered_search_utils.js | 2 + 5 files changed, 148 insertions(+), 26 deletions(-) diff --git a/src/components/base/filtered_search/filtered_search.spec.js b/src/components/base/filtered_search/filtered_search.spec.js index 4d0a70da5a..8db46ae6ba 100644 --- a/src/components/base/filtered_search/filtered_search.spec.js +++ b/src/components/base/filtered_search/filtered_search.spec.js @@ -5,7 +5,7 @@ import GlFilteredSearchSuggestion from './filtered_search_suggestion.vue'; import GlFilteredSearchSuggestionList from './filtered_search_suggestion_list.vue'; import GlFilteredSearchTerm from './filtered_search_term.vue'; import GlFilteredSearchToken from './filtered_search_token.vue'; -import { TERM_TOKEN_TYPE } from './filtered_search_utils'; +import { TERM_TOKEN_TYPE, INTENT_ACTIVATE_PREVIOUS } from './filtered_search_utils'; jest.mock('~/directives/tooltip'); @@ -196,9 +196,99 @@ describe('Filtered search', () => { ]); }); - it('brings focus to previous token if current is destroyed', async () => { + it('makes previous token active if user intends it on token destruction', async () => { createComponent({ - value: ['one', { type: 'faketoken', value: '' }, 'two'], + value: [{ type: 'faketoken', value: '' }, ''], + }); + await nextTick(); + + wrapper + .findComponent(GlFilteredSearchTerm) + .vm.$emit('destroy', { intent: INTENT_ACTIVATE_PREVIOUS }); + + await nextTick(); + + expect(wrapper.findComponent(FakeToken).props('active')).toBe(true); + }); + + it('makes no token active if user intends it on first token destruction', async () => { + createComponent({ + value: ['foo', { type: 'faketoken', value: '' }], + }); + await nextTick(); + wrapper.findComponent(FakeToken).vm.$emit('activate'); + await nextTick(); + + expect(wrapper.findComponent(FakeToken).props('active')).toBe(true); + + wrapper + .findAllComponents(GlFilteredSearchTerm) + .at(0) + .vm.$emit('destroy', { intent: INTENT_ACTIVATE_PREVIOUS }); + + await nextTick(); + + expect(wrapper.findComponent(FakeToken).props('active')).toBe(false); + }); + + it('keeps active token active if later one destroyed', async () => { + createComponent({ + value: [ + { type: 'faketoken', value: '' }, + { type: 'faketoken', value: '' }, + { type: 'faketoken', value: '' }, + ], + }); + await nextTick(); + wrapper.findAllComponents(FakeToken).at(0).vm.$emit('activate'); + await nextTick(); + + wrapper.findAllComponents(FakeToken).at(2).vm.$emit('destroy'); + + await nextTick(); + + expect(wrapper.findAllComponents(FakeToken).at(0).props('active')).toBe(true); + }); + + it('keeps active token active if earlier one destroyed', async () => { + createComponent({ + value: [ + { type: 'faketoken', value: '' }, + { type: 'faketoken', value: '' }, + { type: 'faketoken', value: '' }, + ], + }); + await nextTick(); + wrapper.findAllComponents(FakeToken).at(2).vm.$emit('activate'); + await nextTick(); + + wrapper.findAllComponents(FakeToken).at(0).vm.$emit('destroy'); + + await nextTick(); + + expect(wrapper.findAllComponents(FakeToken).at(1).props('active')).toBe(true); + }); + + it('makes no token active if current is destroyed', async () => { + createComponent({ + value: ['one', { type: 'faketoken', value: '' }], + }); + await nextTick(); + wrapper.findComponent(FakeToken).vm.$emit('activate'); + await nextTick(); + + wrapper.findComponent(FakeToken).vm.$emit('destroy'); + + await nextTick(); + + wrapper.findAllComponents(GlFilteredSearchTerm).wrappers.forEach((searchTermWrapper) => { + expect(searchTermWrapper.props('active')).toBe(false); + }); + }); + + it('keeps no token active if one was destroyed when none were active', async () => { + createComponent({ + value: ['one', { type: 'faketoken', value: '' }], }); await nextTick(); @@ -206,7 +296,7 @@ describe('Filtered search', () => { await nextTick(); - expect(wrapper.findComponent(GlFilteredSearchTerm).props('active')).toBe(true); + expect(wrapper.findComponent(GlFilteredSearchTerm).props('active')).toBe(false); }); it('does not destroy last token', async () => { @@ -568,22 +658,26 @@ describe('Filtered search integration tests', () => { expect(wrapper.findAllComponents(GlFilteredSearchTerm).at(1).find('input').exists()).toBe(true); }); - it('correctly switches focus on token destroy', async () => { - mountComponent({ value: ['one t three'] }); + it('activates previous token when backspacing on empty search term', async () => { + mountComponent({ value: ['zero one two'] }); await nextTick(); activate(1); await nextTick(); - // Unfortunately backspace is not working in JSDOM - wrapper.findAllComponents(GlFilteredSearchTerm).at(1).vm.$emit('destroy'); + // Make sure we have the expected search term + const inputWrapper = wrapper.find('input'); + expect(inputWrapper.element.value).toBe('one'); - await nextTick(); + // Mimic backspace behavior for jsdom + await inputWrapper.setValue(''); + await inputWrapper.trigger('keydown', { key: 'Backspace' }); - expect(document.activeElement).toBe( - wrapper.findComponent(GlFilteredSearchTerm).find('input').element - ); + // Make sure the previous token/search term is now active + const input = wrapper.find('input').element; + expect(input.value).toBe('zero'); + expect(document.activeElement).toBe(input); }); it('clicking clear button clears component input', async () => { diff --git a/src/components/base/filtered_search/filtered_search.vue b/src/components/base/filtered_search/filtered_search.vue index ae379b1611..e7021766a9 100644 --- a/src/components/base/filtered_search/filtered_search.vue +++ b/src/components/base/filtered_search/filtered_search.vue @@ -9,6 +9,7 @@ import GlFilteredSearchTerm from './filtered_search_term.vue'; import { isEmptyTerm, TERM_TOKEN_TYPE, + INTENT_ACTIVATE_PREVIOUS, normalizeTokens, denormalizeTokens, needDenormalization, @@ -201,15 +202,29 @@ export default { this.activeTokenIdx = null; }, - destroyToken(idx) { + destroyToken(idx, { intent } = {}) { if (this.tokens.length === 1) { return; } this.tokens.splice(idx, 1); - if (idx !== 0) { - this.activeTokenIdx = idx - 1; + + // First, attempt to honor the user's activation intent behind the + // destruction of the token, if any. Otherwise, try to maintain the + // active state for the token that was active at the time. If that's not + // possible, make sure no token is active. + if (intent === INTENT_ACTIVATE_PREVIOUS) { + // If there is a previous token, activate it; else, deactivate all tokens + this.activeTokenIdx = idx > 0 ? idx - 1 : null; + } else if (idx < this.activeTokenIdx) { + // Preserve the active token's active status (it shifted down one index) + this.activeTokenIdx -= 1; + } else if (idx === this.activeTokenIdx) { + // User destroyed the active token; don't activate another one. + this.activeTokenIdx = null; } + // Do nothing if there was no active token, or if idx > this.activeTokenIdx, + // to preserve the active state of the remaining tokens. }, replaceToken(idx, token) { @@ -297,7 +312,7 @@ export default { }" @activate="activate(idx)" @deactivate="deactivate(token)" - @destroy="destroyToken(idx)" + @destroy="destroyToken(idx, $event)" @replace="replaceToken(idx, $event)" @complete="completeToken" @submit="submit" diff --git a/src/components/base/filtered_search/filtered_search_term.spec.js b/src/components/base/filtered_search/filtered_search_term.spec.js index f8f4b87cd6..b048452583 100644 --- a/src/components/base/filtered_search/filtered_search_term.spec.js +++ b/src/components/base/filtered_search/filtered_search_term.spec.js @@ -2,6 +2,7 @@ import { nextTick } from 'vue'; import { shallowMount } from '@vue/test-utils'; import GlFilteredSearchSuggestion from './filtered_search_suggestion.vue'; import FilteredSearchTerm from './filtered_search_term.vue'; +import { INTENT_ACTIVATE_PREVIOUS } from './filtered_search_utils'; const availableTokens = [ { type: 'foo', title: 'test1-foo', token: 'stub', icon: 'eye' }, @@ -59,23 +60,27 @@ describe('Filtered search term', () => { }); it.each` - originalEvent | emittedEvent - ${'activate'} | ${'activate'} - ${'deactivate'} | ${'deactivate'} - ${'split'} | ${'split'} - ${'submit'} | ${'submit'} - ${'complete'} | ${'replace'} - ${'backspace'} | ${'destroy'} + originalEvent | emittedEvent | payload + ${'activate'} | ${'activate'} | ${undefined} + ${'deactivate'} | ${'deactivate'} | ${undefined} + ${'split'} | ${'split'} | ${undefined} + ${'submit'} | ${'submit'} | ${undefined} + ${'complete'} | ${'replace'} | ${{ type: undefined }} + ${'backspace'} | ${'destroy'} | ${{ intent: INTENT_ACTIVATE_PREVIOUS }} `( 'emits $emittedEvent when token segment emits $originalEvent', - async ({ originalEvent, emittedEvent }) => { + async ({ originalEvent, emittedEvent, payload }) => { createComponent({ active: true, value: { data: 'something' } }); findTokenSegmentComponent().vm.$emit(originalEvent); await nextTick(); - expect(wrapper.emitted()[emittedEvent]).toHaveLength(1); + expect(wrapper.emitted(emittedEvent)).toHaveLength(1); + + if (payload !== undefined) { + expect(wrapper.emitted(emittedEvent)[0][0]).toEqual(payload); + } } ); diff --git a/src/components/base/filtered_search/filtered_search_term.vue b/src/components/base/filtered_search/filtered_search_term.vue index 9776b4dc21..df03b10765 100644 --- a/src/components/base/filtered_search/filtered_search_term.vue +++ b/src/components/base/filtered_search/filtered_search_term.vue @@ -1,6 +1,7 @@ @@ -69,7 +75,7 @@ export default { @activate="$emit('activate')" @deactivate="$emit('deactivate')" @complete="$emit('replace', { type: $event })" - @backspace="$emit('destroy')" + @backspace="onBackspace" @submit="$emit('submit')" @split="$emit('split', $event)" > diff --git a/src/components/base/filtered_search/filtered_search_utils.js b/src/components/base/filtered_search/filtered_search_utils.js index a6b6b8ed46..9668e77afa 100644 --- a/src/components/base/filtered_search/filtered_search_utils.js +++ b/src/components/base/filtered_search/filtered_search_utils.js @@ -2,6 +2,8 @@ import { first, last, isString } from 'lodash'; export const TERM_TOKEN_TYPE = 'filtered-search-term'; +export const INTENT_ACTIVATE_PREVIOUS = 'intent-activate-previous'; + export function isEmptyTerm(token) { return token.type === TERM_TOKEN_TYPE && token.value.data.trim() === ''; } -- GitLab