From 0583916d2d9ad19ae342a13ff2a31c9e3bb76547 Mon Sep 17 00:00:00 2001 From: winh Date: Thu, 18 May 2017 20:53:14 +0200 Subject: [PATCH] Style people in issuable search bar (!11402) --- .../filtered_search/dropdown_utils.js | 5 +- .../filtered_search_visual_tokens.js | 72 ++++++++-- app/assets/stylesheets/framework/filters.scss | 1 + .../winh-styled-people-search-bar.yml | 4 + spec/features/boards/modal_filter_spec.rb | 4 +- .../filtered_search/filter_issues_spec.rb | 2 +- .../filtered_search/visual_tokens_spec.rb | 4 +- .../filtered_search/dropdown_utils_spec.js | 29 ++++ .../filtered_search_visual_tokens_spec.js | 125 ++++++++++++++++++ .../helpers/filtered_search_spec_helper.js | 13 +- 10 files changed, 237 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/winh-styled-people-search-bar.yml diff --git a/app/assets/javascripts/filtered_search/dropdown_utils.js b/app/assets/javascripts/filtered_search/dropdown_utils.js index 5c02a7a53d3..ef8fe071012 100644 --- a/app/assets/javascripts/filtered_search/dropdown_utils.js +++ b/app/assets/javascripts/filtered_search/dropdown_utils.js @@ -102,10 +102,13 @@ class DropdownUtils { if (token.classList.contains('js-visual-token')) { const name = token.querySelector('.name'); const value = token.querySelector('.value'); + const valueContainer = token.querySelector('.value-container'); const symbol = value && value.dataset.symbol ? value.dataset.symbol : ''; let valueText = ''; - if (value && value.innerText) { + if (valueContainer && valueContainer.dataset.originalValue) { + valueText = valueContainer.dataset.originalValue; + } else if (value && value.innerText) { valueText = value.innerText; } diff --git a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js index bc1226f5879..e9278140af0 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js +++ b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js @@ -1,6 +1,7 @@ -import AjaxCache from '~/lib/utils/ajax_cache'; -import '~/flash'; /* global Flash */ +import AjaxCache from '../lib/utils/ajax_cache'; +import '../flash'; /* global Flash */ import FilteredSearchContainer from './container'; +import UsersCache from '../lib/utils/users_cache'; class FilteredSearchVisualTokens { static getLastVisualTokenBeforeInput() { @@ -82,12 +83,42 @@ class FilteredSearchVisualTokens { .catch(() => new Flash('An error occurred while fetching label colors.')); } + static updateUserTokenAppearance(tokenValueContainer, tokenValueElement, tokenValue) { + if (tokenValue === 'none') { + return Promise.resolve(); + } + + const username = tokenValue.replace(/^@/, ''); + return UsersCache.retrieve(username) + .then((user) => { + if (!user) { + return; + } + + /* eslint-disable no-param-reassign */ + tokenValueContainer.dataset.originalValue = tokenValue; + tokenValueElement.innerHTML = ` + ${user.name}'s avatar + ${user.name} + `; + /* eslint-enable no-param-reassign */ + }) + // ignore error and leave username in the search bar + .catch(() => { }); + } + static renderVisualTokenValue(parentElement, tokenName, tokenValue) { const tokenValueContainer = parentElement.querySelector('.value-container'); - tokenValueContainer.querySelector('.value').innerText = tokenValue; + const tokenValueElement = tokenValueContainer.querySelector('.value'); + tokenValueElement.innerText = tokenValue; - if (tokenName.toLowerCase() === 'label') { + const tokenType = tokenName.toLowerCase(); + if (tokenType === 'label') { FilteredSearchVisualTokens.updateLabelTokenColor(tokenValueContainer, tokenValue); + } else if ((tokenType === 'author') || (tokenType === 'assignee')) { + FilteredSearchVisualTokens.updateUserTokenAppearance( + tokenValueContainer, tokenValueElement, tokenValue, + ); } } @@ -153,6 +184,12 @@ class FilteredSearchVisualTokens { if (!lastVisualToken) return ''; + const valueContainer = lastVisualToken.querySelector('.value-container'); + const originalValue = valueContainer && valueContainer.dataset.originalValue; + if (originalValue) { + return originalValue; + } + const value = lastVisualToken.querySelector('.value'); const name = lastVisualToken.querySelector('.name'); @@ -205,17 +242,28 @@ class FilteredSearchVisualTokens { const inputLi = input.parentElement; tokenContainer.replaceChild(inputLi, token); - const name = token.querySelector('.name'); - const value = token.querySelector('.value'); + const nameElement = token.querySelector('.name'); + let value; - if (token.classList.contains('filtered-search-token') && value) { - FilteredSearchVisualTokens.addFilterVisualToken(name.innerText); - input.value = value.innerText; - } else { - // token is a search term - input.value = name.innerText; + if (token.classList.contains('filtered-search-token')) { + FilteredSearchVisualTokens.addFilterVisualToken(nameElement.innerText); + + const valueContainerElement = token.querySelector('.value-container'); + value = valueContainerElement.dataset.originalValue; + + if (!value) { + const valueElement = valueContainerElement.querySelector('.value'); + value = valueElement.innerText; + } } + // token is a search term + if (!value) { + value = nameElement.innerText; + } + + input.value = value; + // Opens dropdown const inputEvent = new Event('input'); input.dispatchEvent(inputEvent); diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index 52c9c3c88d4..585f4871f5f 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -90,6 +90,7 @@ .filtered-search-term { display: -webkit-flex; display: flex; + flex-shrink: 0; margin-top: 5px; margin-bottom: 5px; diff --git a/changelogs/unreleased/winh-styled-people-search-bar.yml b/changelogs/unreleased/winh-styled-people-search-bar.yml new file mode 100644 index 00000000000..a088af37d8d --- /dev/null +++ b/changelogs/unreleased/winh-styled-people-search-bar.yml @@ -0,0 +1,4 @@ +--- +title: Style people in issuable search bar +merge_request: 11402 +author: diff --git a/spec/features/boards/modal_filter_spec.rb b/spec/features/boards/modal_filter_spec.rb index ce132bfd979..b6de6143354 100644 --- a/spec/features/boards/modal_filter_spec.rb +++ b/spec/features/boards/modal_filter_spec.rb @@ -89,7 +89,7 @@ describe 'Issue Boards add issue modal filtering', :feature, :js do page.within('.add-issues-modal') do wait_for_requests - expect(page).to have_selector('.js-visual-token', text: user2.username) + expect(page).to have_selector('.js-visual-token', text: user2.name) expect(page).to have_selector('.card', count: 1) end end @@ -125,7 +125,7 @@ describe 'Issue Boards add issue modal filtering', :feature, :js do page.within('.add-issues-modal') do wait_for_requests - expect(page).to have_selector('.js-visual-token', text: user2.username) + expect(page).to have_selector('.js-visual-token', text: user2.name) expect(page).to have_selector('.card', count: 1) end end diff --git a/spec/features/issues/filtered_search/filter_issues_spec.rb b/spec/features/issues/filtered_search/filter_issues_spec.rb index 7958ad7e24f..e5e4ba06b5a 100644 --- a/spec/features/issues/filtered_search/filter_issues_spec.rb +++ b/spec/features/issues/filtered_search/filter_issues_spec.rb @@ -6,7 +6,7 @@ describe 'Filter issues', js: true, feature: true do let!(:group) { create(:group) } let!(:project) { create(:project, group: group) } - let!(:user) { create(:user, username: 'joe') } + let!(:user) { create(:user, username: 'joe', name: 'Joe') } let!(:user2) { create(:user, username: 'jane') } let!(:label) { create(:label, project: project) } let!(:wontfix) { create(:label, project: project, title: "Won't fix") } diff --git a/spec/features/issues/filtered_search/visual_tokens_spec.rb b/spec/features/issues/filtered_search/visual_tokens_spec.rb index 96e87c82d2c..dbbafc9e004 100644 --- a/spec/features/issues/filtered_search/visual_tokens_spec.rb +++ b/spec/features/issues/filtered_search/visual_tokens_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' describe 'Visual tokens', js: true, feature: true do include FilteredSearchHelpers + include WaitForRequests let!(:project) { create(:empty_project) } let!(:user) { create(:user, name: 'administrator', username: 'root') } @@ -70,7 +71,8 @@ describe 'Visual tokens', js: true, feature: true do end it 'changes value in visual token' do - expect(first('.tokens-container .filtered-search-token .value').text).to eq("@#{user_rock.username}") + wait_for_requests + expect(first('.tokens-container .filtered-search-token .value').text).to eq("#{user_rock.name}") end it 'moves input to the right' do diff --git a/spec/javascripts/filtered_search/dropdown_utils_spec.js b/spec/javascripts/filtered_search/dropdown_utils_spec.js index bb02abdeea2..f55726379f3 100644 --- a/spec/javascripts/filtered_search/dropdown_utils_spec.js +++ b/spec/javascripts/filtered_search/dropdown_utils_spec.js @@ -2,8 +2,12 @@ import '~/extensions/array'; import '~/filtered_search/dropdown_utils'; import '~/filtered_search/filtered_search_tokenizer'; import '~/filtered_search/filtered_search_dropdown_manager'; +import FilteredSearchSpecHelper from '../helpers/filtered_search_spec_helper'; describe('Dropdown Utils', () => { + const issueListFixture = 'issues/issue_list.html.raw'; + preloadFixtures(issueListFixture); + describe('getEscapedText', () => { it('should return same word when it has no space', () => { const escaped = gl.DropdownUtils.getEscapedText('textWithoutSpace'); @@ -314,4 +318,29 @@ describe('Dropdown Utils', () => { }); }); }); + + describe('getSearchQuery', () => { + let authorToken; + + beforeEach(() => { + loadFixtures(issueListFixture); + + authorToken = FilteredSearchSpecHelper.createFilterVisualToken('author', '@user'); + const searchTermToken = FilteredSearchSpecHelper.createSearchVisualToken('search term'); + + const tokensContainer = document.querySelector('.tokens-container'); + tokensContainer.appendChild(searchTermToken); + tokensContainer.appendChild(authorToken); + }); + + it('uses original value if present', () => { + const originalValue = 'original dance'; + const valueContainer = authorToken.querySelector('.value-container'); + valueContainer.dataset.originalValue = originalValue; + + const searchQuery = gl.DropdownUtils.getSearchQuery(); + + expect(searchQuery).toBe(' search term author:original dance'); + }); + }); }); diff --git a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js index 39df072573e..fa4343ffbc8 100644 --- a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js +++ b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js @@ -1,4 +1,5 @@ import AjaxCache from '~/lib/utils/ajax_cache'; +import UsersCache from '~/lib/utils/users_cache'; import '~/filtered_search/filtered_search_visual_tokens'; import FilteredSearchSpecHelper from '../helpers/filtered_search_spec_helper'; @@ -406,6 +407,22 @@ describe('Filtered Search Visual Tokens', () => { expect(subject.getLastTokenPartial()).toEqual(value); }); + it('should get last token original value if available', () => { + const originalValue = '@user'; + const valueContainer = authorToken.querySelector('.value-container'); + valueContainer.dataset.originalValue = originalValue; + const avatar = document.createElement('img'); + const valueElement = valueContainer.querySelector('.value'); + valueElement.insertAdjacentElement('afterbegin', avatar); + tokensContainer.innerHTML = FilteredSearchSpecHelper.createTokensContainerHTML( + authorToken.outerHTML, + ); + + const lastTokenValue = subject.getLastTokenPartial(); + + expect(lastTokenValue).toEqual(originalValue); + }); + it('should get last token name if there is no value', () => { const name = 'assignee'; tokensContainer.innerHTML = FilteredSearchSpecHelper.createTokensContainerHTML( @@ -534,6 +551,16 @@ describe('Filtered Search Visual Tokens', () => { expect(input.value).toEqual('none'); }); + it('input contains the original value if present', () => { + const originalValue = '@user'; + const valueContainer = token.querySelector('.value-container'); + valueContainer.dataset.originalValue = originalValue; + + subject.editToken(token); + + expect(input.value).toEqual(originalValue); + }); + describe('selected token is a search term token', () => { beforeEach(() => { token = document.querySelector('.filtered-search-term'); @@ -633,6 +660,7 @@ describe('Filtered Search Visual Tokens', () => { const milestoneToken = FilteredSearchSpecHelper.createFilterVisualToken('milestone', 'upcoming'); let updateLabelTokenColorSpy; + let updateUserTokenAppearanceSpy; beforeEach(() => { tokensContainer.innerHTML = FilteredSearchSpecHelper.createTokensContainerHTML(` @@ -644,6 +672,24 @@ describe('Filtered Search Visual Tokens', () => { spyOn(subject, 'updateLabelTokenColor'); updateLabelTokenColorSpy = subject.updateLabelTokenColor; + + spyOn(subject, 'updateUserTokenAppearance'); + updateUserTokenAppearanceSpy = subject.updateUserTokenAppearance; + }); + + it('renders a author token value element', () => { + const { tokenNameElement, tokenValueContainer, tokenValueElement } = + findElements(authorToken); + const tokenName = tokenNameElement.innerText; + const tokenValue = 'new value'; + + subject.renderVisualTokenValue(authorToken, tokenName, tokenValue); + + expect(tokenValueElement.innerText).toBe(tokenValue); + expect(updateUserTokenAppearanceSpy.calls.count()).toBe(1); + const expectedArgs = [tokenValueContainer, tokenValueElement, tokenValue]; + expect(updateUserTokenAppearanceSpy.calls.argsFor(0)).toEqual(expectedArgs); + expect(updateLabelTokenColorSpy.calls.count()).toBe(0); }); it('renders a label token value element', () => { @@ -658,6 +704,7 @@ describe('Filtered Search Visual Tokens', () => { expect(updateLabelTokenColorSpy.calls.count()).toBe(1); const expectedArgs = [tokenValueContainer, tokenValue]; expect(updateLabelTokenColorSpy.calls.argsFor(0)).toEqual(expectedArgs); + expect(updateUserTokenAppearanceSpy.calls.count()).toBe(0); }); it('renders a milestone token value element', () => { @@ -669,6 +716,84 @@ describe('Filtered Search Visual Tokens', () => { expect(tokenValueElement.innerText).toBe(tokenValue); expect(updateLabelTokenColorSpy.calls.count()).toBe(0); + expect(updateUserTokenAppearanceSpy.calls.count()).toBe(0); + }); + }); + + describe('updateUserTokenAppearance', () => { + let usersCacheSpy; + + beforeEach(() => { + spyOn(UsersCache, 'retrieve').and.callFake(username => usersCacheSpy(username)); + }); + + it('ignores special value "none"', (done) => { + usersCacheSpy = (username) => { + expect(username).toBe('none'); + done.fail('Should not resolve "none"!'); + }; + const { tokenValueContainer, tokenValueElement } = findElements(authorToken); + + subject.updateUserTokenAppearance(tokenValueContainer, tokenValueElement, 'none') + .then(done) + .catch(done.fail); + }); + + it('ignores error if UsersCache throws', (done) => { + spyOn(window, 'Flash'); + const dummyError = new Error('Earth rotated backwards'); + const { tokenValueContainer, tokenValueElement } = findElements(authorToken); + const tokenValue = tokenValueElement.innerText; + usersCacheSpy = (username) => { + expect(`@${username}`).toBe(tokenValue); + return Promise.reject(dummyError); + }; + + subject.updateUserTokenAppearance(tokenValueContainer, tokenValueElement, tokenValue) + .then(() => { + expect(window.Flash.calls.count()).toBe(0); + }) + .then(done) + .catch(done.fail); + }); + + it('does nothing if user cannot be found', (done) => { + const { tokenValueContainer, tokenValueElement } = findElements(authorToken); + const tokenValue = tokenValueElement.innerText; + usersCacheSpy = (username) => { + expect(`@${username}`).toBe(tokenValue); + return Promise.resolve(undefined); + }; + + subject.updateUserTokenAppearance(tokenValueContainer, tokenValueElement, tokenValue) + .then(() => { + expect(tokenValueElement.innerText).toBe(tokenValue); + }) + .then(done) + .catch(done.fail); + }); + + it('replaces author token with avatar and display name', (done) => { + const dummyUser = { + name: 'Important Person', + avatar_url: 'https://host.invalid/mypics/avatar.png', + }; + const { tokenValueContainer, tokenValueElement } = findElements(authorToken); + const tokenValue = tokenValueElement.innerText; + usersCacheSpy = (username) => { + expect(`@${username}`).toBe(tokenValue); + return Promise.resolve(dummyUser); + }; + + subject.updateUserTokenAppearance(tokenValueContainer, tokenValueElement, tokenValue) + .then(() => { + expect(tokenValueContainer.dataset.originalValue).toBe(tokenValue); + expect(tokenValueElement.innerText.trim()).toBe(dummyUser.name); + const avatar = tokenValueElement.querySelector('img.avatar'); + expect(avatar.src).toBe(dummyUser.avatar_url); + }) + .then(done) + .catch(done.fail); }); }); diff --git a/spec/javascripts/helpers/filtered_search_spec_helper.js b/spec/javascripts/helpers/filtered_search_spec_helper.js index 0d7092a2357..8933dd5def4 100644 --- a/spec/javascripts/helpers/filtered_search_spec_helper.js +++ b/spec/javascripts/helpers/filtered_search_spec_helper.js @@ -30,12 +30,15 @@ export default class FilteredSearchSpecHelper { `; } + static createSearchVisualToken(name) { + const li = document.createElement('li'); + li.classList.add('js-visual-token', 'filtered-search-term'); + li.innerHTML = `
${name}
`; + return li; + } + static createSearchVisualTokenHTML(name) { - return ` -
  • -
    ${name}
    -
    • - `; + return FilteredSearchSpecHelper.createSearchVisualToken(name).outerHTML; } static createInputHTML(placeholder = '', value = '') {