From 3483beef0ae6159d55b5e2112d8a1915e133800c Mon Sep 17 00:00:00 2001 From: dan-searle Date: Thu, 24 Jan 2019 13:48:07 +0000 Subject: [PATCH 01/13] Extend Component class instead of using x-interaction --- components/x-topic-search/package.json | 1 - .../x-topic-search/src/ResultContainer.jsx | 10 +- components/x-topic-search/src/TopicSearch.jsx | 154 +++++++++--------- 3 files changed, 84 insertions(+), 81 deletions(-) diff --git a/components/x-topic-search/package.json b/components/x-topic-search/package.json index fd441f3b8..22c52604e 100644 --- a/components/x-topic-search/package.json +++ b/components/x-topic-search/package.json @@ -19,7 +19,6 @@ "dependencies": { "@financial-times/x-engine": "file:../../packages/x-engine", "@financial-times/x-follow-button": "0.0.11", - "@financial-times/x-interaction": "^1.0.0-beta.6", "classnames": "^2.2.6", "debounce-promise": "^3.1.0" }, diff --git a/components/x-topic-search/src/ResultContainer.jsx b/components/x-topic-search/src/ResultContainer.jsx index d696567aa..d360cf75e 100644 --- a/components/x-topic-search/src/ResultContainer.jsx +++ b/components/x-topic-search/src/ResultContainer.jsx @@ -29,24 +29,24 @@ const arrayToSentence = followedSuggestions => { }; -export default ({ result, searchTerm, csrfToken, followedTopicIds }) => { +export default ({ followedSuggestions, searchTerm, csrfToken, followedTopicIds, unfollowedSuggestions }) => { - const hasFollowedSuggestions = result.followedSuggestions.length > 0; - const hasUnfollowedSuggestions = result.unfollowedSuggestions.length > 0; + const hasFollowedSuggestions = followedSuggestions.length > 0; + const hasUnfollowedSuggestions = unfollowedSuggestions.length > 0; return (
{ hasUnfollowedSuggestions && } { !hasUnfollowedSuggestions && hasFollowedSuggestions &&
- You already follow { arrayToSentence(result.followedSuggestions) } + You already follow { arrayToSentence(followedSuggestions) }
} { !hasUnfollowedSuggestions && !hasFollowedSuggestions && diff --git a/components/x-topic-search/src/TopicSearch.jsx b/components/x-topic-search/src/TopicSearch.jsx index 4a5e93a9c..cb00d8cbd 100644 --- a/components/x-topic-search/src/TopicSearch.jsx +++ b/components/x-topic-search/src/TopicSearch.jsx @@ -1,5 +1,4 @@ -import { h } from '@financial-times/x-engine'; -import { withActions } from '@financial-times/x-interaction'; +import { h, Component } from '@financial-times/x-engine'; import styles from './TopicSearch.scss'; import classNames from 'classnames'; import getSuggestions from './lib/get-suggestions.js'; @@ -7,89 +6,94 @@ import debounce from 'debounce-promise'; import ResultContainer from './ResultContainer'; -const debounceGetSuggestions = debounce(getSuggestions, 150); +class TopicSearch extends Component { + constructor(props) { + super(props); + + this.minSearchLength = props.minSearchLength || 2; + this.maxSuggestions = props.maxSuggestions || 5; + this.apiUrl = props.apiUrl; + this.getSuggestions = debounce(getSuggestions, 150); + this.handleInputChange = this.handleInputChange.bind(this); + this.handleInputClickOrFocus = this.handleInputClickOrFocus.bind(this); + + this.state = { + followedTopicIds: [], + searchTerm: '', + showResult: false, + followedSuggestions: [], + unFollowedSuggestions: [] + }; + } -let resultExists = false; + handleInputChange(event) { + const searchTerm = event.target.value.trim(); -const topicSearchActions = withActions(({ minSearchLength = 2, maxSuggestions = 5, apiUrl, followedTopicIds = [] }) => ({ - async checkInput(event) { - const searchTerm = event.target.value && event.target.value.trim(); + this.setState({ searchTerm }); - if (searchTerm.length >= minSearchLength) { - return debounceGetSuggestions(searchTerm, maxSuggestions, apiUrl, followedTopicIds) - .then(result => { - resultExists = true; - return { showResult: true, result, searchTerm }; + if (searchTerm.length >= this.minSearchLength) { + this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl, this.state.followedTopicIds) + .then(({ followedSuggestions, unfollowedSuggestions }) => { + this.setState({ + followedSuggestions, + unfollowedSuggestions, + showResult: true + }); }) .catch(() => { - resultExists = false; - return { showResult: false }; + this.setState({ + showResult: false + }); }); } else { - resultExists = false; - return Promise.resolve({ showResult: false }); - } - }, - - topicFollowed (subjectId) { - if (!followedTopicIds.includes(subjectId)) { - followedTopicIds.push(subjectId); - } - - return { followedTopicIds }; - }, - - topicUnfollowed (subjectId) { - const targetIdIndex = followedTopicIds.indexOf(subjectId); - - if (targetIdIndex > -1) { - followedTopicIds.splice(targetIdIndex, 1); + this.setState({ + showResult: false + }); } - - return { followedTopicIds }; - }, - - selectInput (event) { - event.target.select(); - return { showResult: resultExists }; - }, - - hideResult() { - return { showResult: false }; } -})); - -const TopicSearch = topicSearchActions(({ searchTerm, showResult, result, actions, isLoading, csrfToken, followedTopicIds }) => ( -
-

- Search for topics, authors, companies, or other areas of interest -

- -
- - -
- - { showResult && !isLoading && - } + handleInputClickOrFocus() { + console.log('handleInputClickOrFocus'); + // this.setState({ + // showResult: true + // }); + } -
-)); + render() { + const { csrfToken, followedSuggestions, followedTopicIds, isLoading, searchTerm, showResult, unfollowedSuggestions } = this.state; + + return ( +
+

+ Search for topics, authors, companies, or other areas of interest +

+ + +
+ + +
+ + { showResult && !isLoading && + } +
+ ); + } +} export { TopicSearch }; From 2070f5c43a54cbd1ccb8bcf3f60857a3b71cb65e Mon Sep 17 00:00:00 2001 From: dan-searle Date: Mon, 28 Jan 2019 16:50:56 +0000 Subject: [PATCH 02/13] followedTopicIds prop is expected to be updated when followed topics change. --- components/x-topic-search/src/TopicSearch.jsx | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/components/x-topic-search/src/TopicSearch.jsx b/components/x-topic-search/src/TopicSearch.jsx index cb00d8cbd..284489135 100644 --- a/components/x-topic-search/src/TopicSearch.jsx +++ b/components/x-topic-search/src/TopicSearch.jsx @@ -14,25 +14,38 @@ class TopicSearch extends Component { this.maxSuggestions = props.maxSuggestions || 5; this.apiUrl = props.apiUrl; this.getSuggestions = debounce(getSuggestions, 150); + this.outsideEvents = ['focusout', 'focusin', 'click']; this.handleInputChange = this.handleInputChange.bind(this); this.handleInputClickOrFocus = this.handleInputClickOrFocus.bind(this); + this.handleInteractionOutside = this.handleInteractionOutside.bind(this); this.state = { - followedTopicIds: [], searchTerm: '', showResult: false, followedSuggestions: [], - unFollowedSuggestions: [] + unfollowedSuggestions: [] }; } + componentDidMount() { + this.outsideEvents.forEach(action => { + document.body.addEventListener(action, this.handleInteractionOutside); + }); + } + + componentWillUnmount() { + this.outsideEvents.forEach(action => { + document.body.removeEventListener(action, this.handleInteractionOutside); + }); + } + handleInputChange(event) { const searchTerm = event.target.value.trim(); this.setState({ searchTerm }); if (searchTerm.length >= this.minSearchLength) { - this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl, this.state.followedTopicIds) + this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl, this.props.followedTopicIds) .then(({ followedSuggestions, unfollowedSuggestions }) => { this.setState({ followedSuggestions, @@ -52,18 +65,28 @@ class TopicSearch extends Component { } } + handleInteractionOutside(event) { + if (!this.rootEl.contains(event.target)) { + this.setState({ + showResult: false + }); + } + } + handleInputClickOrFocus() { - console.log('handleInputClickOrFocus'); - // this.setState({ - // showResult: true - // }); + if (this.state.searchTerm.length >= this.minSearchLength) { + this.setState({ + showResult: true + }); + } } render() { - const { csrfToken, followedSuggestions, followedTopicIds, isLoading, searchTerm, showResult, unfollowedSuggestions } = this.state; + const { followedTopicIds } = this.props; + const { csrfToken, followedSuggestions, searchTerm, showResult, unfollowedSuggestions } = this.state; return ( -
+
this.rootEl = el}>

Search for topics, authors, companies, or other areas of interest

@@ -84,7 +107,7 @@ class TopicSearch extends Component { />
- { showResult && !isLoading && + { showResult && Date: Tue, 29 Jan 2019 15:49:14 +0000 Subject: [PATCH 03/13] Support class components in story snapshot tests. --- __tests__/snapshots.test.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/__tests__/snapshots.test.js b/__tests__/snapshots.test.js index c802a987c..e32e029a7 100644 --- a/__tests__/snapshots.test.js +++ b/__tests__/snapshots.test.js @@ -2,6 +2,7 @@ const renderer = require('react-test-renderer'); const fs = require('fs'); const path = require('path'); const glob = require('glob'); +const { h } = require('../packages/x-engine'); const {packages} = require('../monorepo.json'); @@ -16,16 +17,16 @@ for(const pkg of packageDirs) { const storiesDir = path.resolve(pkgDir, 'stories'); if(fs.existsSync(storiesDir)) { - const { package: pkg, stories, component } = require(storiesDir); - const { presets = {default: {}} } = require(pkgDir); + const { package: pkg, stories, component: Component } = require(storiesDir); + const { presets = { default: {} } } = require(pkgDir); const name = path.basename(pkg.name); describe(pkg.name, () => { for (const { title, data } of stories) { - for (const [ preset, options ] of Object.entries(presets)) { + for (const [preset, options] of Object.entries(presets)) { it(`renders a ${preset} ${title} ${name}`, () => { const props = { ...data, ...options }; - const tree = renderer.create(component(props)).toJSON(); + const tree = renderer.create(h(Component, props)).toJSON(); expect(tree).toMatchSnapshot(); }); } From d69c8947c227c20b6fcc00f889291491362a3b68 Mon Sep 17 00:00:00 2001 From: dan-searle Date: Wed, 30 Jan 2019 15:57:00 +0000 Subject: [PATCH 04/13] Create story knob for followedTopicIds prop. --- components/x-topic-search/stories/index.js | 3 +++ components/x-topic-search/stories/knobs.js | 19 +++++++++++++++++++ .../x-topic-search/stories/topic-search.js | 6 +++++- 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 components/x-topic-search/stories/knobs.js diff --git a/components/x-topic-search/stories/index.js b/components/x-topic-search/stories/index.js index 419ff844a..601676cb3 100644 --- a/components/x-topic-search/stories/index.js +++ b/components/x-topic-search/stories/index.js @@ -15,3 +15,6 @@ exports.dependencies = { exports.stories = [ require('./topic-search') ]; + +exports.knobs = require('./knobs'); + diff --git a/components/x-topic-search/stories/knobs.js b/components/x-topic-search/stories/knobs.js new file mode 100644 index 000000000..7809db53e --- /dev/null +++ b/components/x-topic-search/stories/knobs.js @@ -0,0 +1,19 @@ +module.exports = (data, { select }) => { + return { + followedTopicIds() { + return select( + 'Followed Topics', + { + None: [], + 'World Elephant Water Polo': ['f95d1e16-2307-4feb-b3ff-6f224798aa49'], + 'Brexit, Brexit Briefing, Brexit Unspun Podcast': [ + '19b95057-4614-45fb-9306-4d54049354db', + '464cc2f2-395e-4c36-bb29-01727fc95558', + 'c4e899ed-157e-4446-86f0-5a65803dc07a' + ] + }, + [] + ); + } + }; +}; diff --git a/components/x-topic-search/stories/topic-search.js b/components/x-topic-search/stories/topic-search.js index 76f3536de..387b4f604 100644 --- a/components/x-topic-search/stories/topic-search.js +++ b/components/x-topic-search/stories/topic-search.js @@ -1,6 +1,6 @@ exports.title = 'Topic Search Bar'; -exports.data = { +const data = { minSearchLength: 2, maxSuggestions: 10, apiUrl: '//tag-facets-api.ft.com/annotations', @@ -10,6 +10,10 @@ exports.data = { csrfToken: 'csrfToken' }; +exports.data = data; + +exports.knobs = Object.keys(data); + // This reference is only required for hot module loading in development // exports.m = module; From 3522bf1b2ff04f4a7eb03270844ef8f96cc9627f Mon Sep 17 00:00:00 2001 From: dan-searle Date: Wed, 30 Jan 2019 16:27:26 +0000 Subject: [PATCH 05/13] Minor improvement to arrayToSentence function. --- .../x-topic-search/src/ResultContainer.jsx | 48 +++++++------------ .../x-topic-search/src/lib/get-suggestions.js | 8 ++-- 2 files changed, 21 insertions(+), 35 deletions(-) diff --git a/components/x-topic-search/src/ResultContainer.jsx b/components/x-topic-search/src/ResultContainer.jsx index d360cf75e..cc6906168 100644 --- a/components/x-topic-search/src/ResultContainer.jsx +++ b/components/x-topic-search/src/ResultContainer.jsx @@ -9,49 +9,37 @@ import NoSuggestions from './NoSuggestions'; const arrayToSentence = followedSuggestions => { const topicsLength = followedSuggestions.length; - if (topicsLength === 1) { - return { followedSuggestions[0].prefLabel }; - } else { - return followedSuggestions - .map((topic, index) => { - if (index === topicsLength - 1) { - // the last topic - return and { topic.prefLabel } - } else if (index === topicsLength - 2) { - // one before the last topic - return { topic.prefLabel } ; - } else { - return { topic.prefLabel }, ; - } - }) - } - + return followedSuggestions.map((topic, index) => ( + + {topicsLength > 1 && index === topicsLength - 1 && ' and '} + {topic.prefLabel} + {index < topicsLength - 2 && ', '} + + )); }; export default ({ followedSuggestions, searchTerm, csrfToken, followedTopicIds, unfollowedSuggestions }) => { - const hasFollowedSuggestions = followedSuggestions.length > 0; const hasUnfollowedSuggestions = unfollowedSuggestions.length > 0; return (
- - { hasUnfollowedSuggestions && + {hasUnfollowedSuggestions && } + suggestions={ unfollowedSuggestions } + searchTerm={ searchTerm } + csrfToken={ csrfToken } + followedTopicIds={ followedTopicIds } + />} - { !hasUnfollowedSuggestions && hasFollowedSuggestions && + {!hasUnfollowedSuggestions && hasFollowedSuggestions &&
- You already follow { arrayToSentence(followedSuggestions) } -
} - - { !hasUnfollowedSuggestions && !hasFollowedSuggestions && - } + You already follow { arrayToSentence(followedSuggestions) } +
} + {!hasUnfollowedSuggestions && !hasFollowedSuggestions && + }
); }; diff --git a/components/x-topic-search/src/lib/get-suggestions.js b/components/x-topic-search/src/lib/get-suggestions.js index 4e1ea8e02..ec21c44ce 100644 --- a/components/x-topic-search/src/lib/get-suggestions.js +++ b/components/x-topic-search/src/lib/get-suggestions.js @@ -8,10 +8,9 @@ const separateFollowedAndUnfollowed = (suggestions = [], followedTopicIds) => { const unfollowedSuggestions = suggestions.filter(suggestion => !followedTopicIds.includes(suggestion.id)); return { followedSuggestions, unfollowedSuggestions }; -} +}; export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { - const dataSrc = addQueryParamToUrl('count', maxSuggestions, apiUrl, false); const url = addQueryParamToUrl('partial', searchTerm.replace(' ', '+'), dataSrc); @@ -25,8 +24,7 @@ export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { .then(suggestions => { return separateFollowedAndUnfollowed(suggestions, followedTopicIds) }) - .catch(() => { - throw new Error(); + .catch((err) => { + throw new Error(err); }); - }; From a49762a536b80fb964e95488f34c4148a56f242e Mon Sep 17 00:00:00 2001 From: dan-searle Date: Wed, 30 Jan 2019 16:43:04 +0000 Subject: [PATCH 06/13] Ensure results are only displayed if they are for the current search term (defends against slow API response causing results to be shown when input is now empty). --- components/x-topic-search/src/TopicSearch.jsx | 9 ++++++--- components/x-topic-search/src/lib/get-suggestions.js | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/components/x-topic-search/src/TopicSearch.jsx b/components/x-topic-search/src/TopicSearch.jsx index 284489135..fc95586bb 100644 --- a/components/x-topic-search/src/TopicSearch.jsx +++ b/components/x-topic-search/src/TopicSearch.jsx @@ -46,8 +46,9 @@ class TopicSearch extends Component { if (searchTerm.length >= this.minSearchLength) { this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl, this.props.followedTopicIds) - .then(({ followedSuggestions, unfollowedSuggestions }) => { + .then(({ resultsForTerm, followedSuggestions, unfollowedSuggestions }) => { this.setState({ + resultsForTerm, followedSuggestions, unfollowedSuggestions, showResult: true @@ -55,11 +56,13 @@ class TopicSearch extends Component { }) .catch(() => { this.setState({ + resultsForTerm: null, showResult: false }); }); } else { this.setState({ + resultsForTerm: null, showResult: false }); } @@ -83,7 +86,7 @@ class TopicSearch extends Component { render() { const { followedTopicIds } = this.props; - const { csrfToken, followedSuggestions, searchTerm, showResult, unfollowedSuggestions } = this.state; + const { csrfToken, followedSuggestions, resultsForTerm, searchTerm, showResult, unfollowedSuggestions } = this.state; return (
this.rootEl = el}> @@ -107,7 +110,7 @@ class TopicSearch extends Component { />
- { showResult && + { showResult && resultsForTerm === searchTerm && { const queryParam = `${name}=${value}`; + return append === true ? `${url}&${queryParam}` : `${url}?${queryParam}`; }; @@ -19,11 +20,13 @@ export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { if (!response.ok) { throw new Error(response.statusText); } + return response.json(); }) - .then(suggestions => { - return separateFollowedAndUnfollowed(suggestions, followedTopicIds) - }) + .then(suggestions => ({ + resultsForTerm: searchTerm, + ...separateFollowedAndUnfollowed(suggestions, followedTopicIds) + })) .catch((err) => { throw new Error(err); }); From b07d8aff5b4bc1ad3fcd3e760b133d2b33c1f37d Mon Sep 17 00:00:00 2001 From: dan-searle Date: Thu, 31 Jan 2019 10:12:54 +0000 Subject: [PATCH 07/13] Simplify get-suggestions response processing. --- components/x-topic-search/src/lib/get-suggestions.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/components/x-topic-search/src/lib/get-suggestions.js b/components/x-topic-search/src/lib/get-suggestions.js index 17b33a398..881b8fefe 100644 --- a/components/x-topic-search/src/lib/get-suggestions.js +++ b/components/x-topic-search/src/lib/get-suggestions.js @@ -4,13 +4,6 @@ const addQueryParamToUrl = (name, value, url, append = true) => { return append === true ? `${url}&${queryParam}` : `${url}?${queryParam}`; }; -const separateFollowedAndUnfollowed = (suggestions = [], followedTopicIds) => { - const followedSuggestions = suggestions.filter(suggestion => followedTopicIds.includes(suggestion.id)); - const unfollowedSuggestions = suggestions.filter(suggestion => !followedTopicIds.includes(suggestion.id)); - - return { followedSuggestions, unfollowedSuggestions }; -}; - export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { const dataSrc = addQueryParamToUrl('count', maxSuggestions, apiUrl, false); const url = addQueryParamToUrl('partial', searchTerm.replace(' ', '+'), dataSrc); @@ -25,7 +18,8 @@ export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { }) .then(suggestions => ({ resultsForTerm: searchTerm, - ...separateFollowedAndUnfollowed(suggestions, followedTopicIds) + followedSuggestions: suggestions.filter(suggestion => followedTopicIds.includes(suggestion.id)), + unfollowedSuggestions: suggestions.filter(suggestion => !followedTopicIds.includes(suggestion.id)) })) .catch((err) => { throw new Error(err); From 2a6f274bc15bb2ac921ebd97f8c008313deb2c15 Mon Sep 17 00:00:00 2001 From: dan-searle Date: Thu, 31 Jan 2019 12:09:20 +0000 Subject: [PATCH 08/13] Slightly more descriptive tests. --- .../__tests__/x-topic-search.test.jsx | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/components/x-topic-search/__tests__/x-topic-search.test.jsx b/components/x-topic-search/__tests__/x-topic-search.test.jsx index c77dc3a30..5bc356fbc 100644 --- a/components/x-topic-search/__tests__/x-topic-search.test.jsx +++ b/components/x-topic-search/__tests__/x-topic-search.test.jsx @@ -14,10 +14,14 @@ const alreadyFollowedTopics = [ { uuid: 'Cat-Food-id', name: 'Cat Food' }, { uuid: 'Cat-Toys-id', name: 'Cat Toys' } ]; -const buildSearchUrl = term => `${apiUrl}?count=${maxSuggestions}&partial=${term}`; describe('x-topic-search', () => { - const waitForApiResponse = () => new Promise(resolve => { setTimeout(resolve, 500); }); + const buildSearchUrl = term => `${apiUrl}?count=${maxSuggestions}&partial=${term}`; + const enterSearchTerm = searchTerm => { + target.find('input').simulate('input', { target: { value: searchTerm }}); + + return new Promise(resolve => { setTimeout(resolve, 400); }); + }; let target; beforeEach(() => { @@ -45,53 +49,54 @@ describe('x-topic-search', () => { }); describe('given inputted text is shorter than minSearchLength', () => { - it('should not render result', () => { - const wordLessThanMin = searchTerm.slice(0, minSearchLength - 1); - const apiUrlWithResults = buildSearchUrl(wordLessThanMin); + const apiUrlWithResults = buildSearchUrl('a'); - fetchMock.get(apiUrlWithResults, []); + fetchMock.get(apiUrlWithResults, []); - target.find('input').simulate('input', { target: { value: wordLessThanMin }}); + beforeEach(() => { + return enterSearchTerm('a'); + }); - return waitForApiResponse().then(() => { - expect(fetchMock.called(apiUrlWithResults)).toBe(false); - expect(target.render().children('div')).toHaveLength(1); - }); + it('does not make a request to the api or render any result', () => { + expect(fetchMock.called(apiUrlWithResults)).toBe(false); + expect(target.render().children('div')).toHaveLength(1); }); }); describe('given searchTerm which has some topic suggestions to follow', () => { const apiUrlWithResults = buildSearchUrl(searchTerm); - const topicSuggestions = [ + const unfollowedTopicSuggestions = [ { id: 'Dog-House-id', prefLabel: 'Dog House', url: 'Dog-House-url' }, { id: 'Dog-Food-id', prefLabel: 'Dog Food', url: 'Dog-Food-url' }, { id: 'Dog-Toys-id', prefLabel: 'Dog Toys', url: 'Dog-Toys-url' } ]; - fetchMock.get(apiUrlWithResults, topicSuggestions); + fetchMock.get(apiUrlWithResults, unfollowedTopicSuggestions); beforeEach(() => { - target.find('input').simulate('input', { target: { value: searchTerm } }); - - return waitForApiResponse(); + return enterSearchTerm(searchTerm); }); - it('should render topics list with follow button', () => { + it('requests the topic suggestions with count set to maxSuggestions', () => { expect(fetchMock.called(apiUrlWithResults)).toBe(true); + }); + + it('renders no more than the max number of suggestions', () => { expect(target.render().children('div')).toHaveLength(2); + expect(target.render().find('li')).toHaveLength(maxSuggestions); + }); + it('renders links and follow buttons for each suggestion', () => { const suggestionsList = target.render().find('li'); - expect(suggestionsList).toHaveLength(maxSuggestions); - - topicSuggestions.forEach((topic, index) => { + unfollowedTopicSuggestions.forEach((topic, index) => { const suggestion = suggestionsList.eq(index); expect(suggestion.find('a').text()).toEqual(topic.prefLabel); expect(suggestion.find('a').attr('href')).toEqual(topic.url); expect(suggestion.find('button')).toHaveLength(1); }); - }); + }) }); describe('given searchTerm which has no topic suggestions to follow', () => { @@ -100,12 +105,10 @@ describe('x-topic-search', () => { fetchMock.get(apiUrlNoResults, []); beforeEach(() => { - target.find('input').simulate('input', { target: { value: searchTermNoResult } }); - - return waitForApiResponse(); + return enterSearchTerm(searchTermNoResult); }); - it('should render no topic message', () => { + it('requests from the api and renders the no matching topics message', () => { expect(fetchMock.called(apiUrlNoResults)).toBe(true); const resultContainer = target.render().children('div').eq(1); @@ -115,7 +118,7 @@ describe('x-topic-search', () => { }); }); - describe('given searchTerm which all the topics has been followed', () => { + describe('given searchTerm which results in all suggestions already followed', () => { const apiUrlAllFollowed = buildSearchUrl(searchTermAllFollowed); fetchMock.get(apiUrlAllFollowed, alreadyFollowedTopics.map(topic => ({ @@ -125,22 +128,19 @@ describe('x-topic-search', () => { }))); beforeEach(() => { - target.find('input').simulate('input', { target: { value: searchTermAllFollowed } }); - - return waitForApiResponse(); + return enterSearchTerm(searchTermAllFollowed); }); - it('should render already followed message with name of the topics', () => { + it('requests the suggestions from the api', () => { expect(fetchMock.called(apiUrlAllFollowed)).toBe(true); + }); + it('renders the "already followed" message with names of the topics', () => { const resultContainer = target.render().children('div').eq(1); expect(resultContainer).toHaveLength(1); expect(resultContainer.text()) - .toMatch( - `You already follow ${alreadyFollowedTopics[0].name}, ${alreadyFollowedTopics[1].name} and ${alreadyFollowedTopics[2].name}` - ); + .toMatch(`You already follow ${alreadyFollowedTopics[0].name}, ${alreadyFollowedTopics[1].name} and ${alreadyFollowedTopics[2].name}`); }); }); - }); From 5db84ab7ec4c8de1676e8c93b10a5fbc9aa44a5c Mon Sep 17 00:00:00 2001 From: dan-searle Date: Thu, 31 Jan 2019 14:20:40 +0000 Subject: [PATCH 09/13] Remove redundant throw in catch. --- components/x-topic-search/src/lib/get-suggestions.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/components/x-topic-search/src/lib/get-suggestions.js b/components/x-topic-search/src/lib/get-suggestions.js index 881b8fefe..f2e89c1c7 100644 --- a/components/x-topic-search/src/lib/get-suggestions.js +++ b/components/x-topic-search/src/lib/get-suggestions.js @@ -20,8 +20,5 @@ export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { resultsForTerm: searchTerm, followedSuggestions: suggestions.filter(suggestion => followedTopicIds.includes(suggestion.id)), unfollowedSuggestions: suggestions.filter(suggestion => !followedTopicIds.includes(suggestion.id)) - })) - .catch((err) => { - throw new Error(err); - }); + })); }; From 3c4523f56e221d6600d2dcc3dc89ec1dd86b59aa Mon Sep 17 00:00:00 2001 From: dan-searle Date: Thu, 31 Jan 2019 15:38:54 +0000 Subject: [PATCH 10/13] Bug fix: csrfToken is a prop not state... --- components/x-topic-search/src/TopicSearch.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/x-topic-search/src/TopicSearch.jsx b/components/x-topic-search/src/TopicSearch.jsx index fc95586bb..6cc5ef522 100644 --- a/components/x-topic-search/src/TopicSearch.jsx +++ b/components/x-topic-search/src/TopicSearch.jsx @@ -85,8 +85,8 @@ class TopicSearch extends Component { } render() { - const { followedTopicIds } = this.props; - const { csrfToken, followedSuggestions, resultsForTerm, searchTerm, showResult, unfollowedSuggestions } = this.state; + const { csrfToken, followedTopicIds } = this.props; + const { followedSuggestions, resultsForTerm, searchTerm, showResult, unfollowedSuggestions } = this.state; return (
this.rootEl = el}> From 69ed7ad2cf0fdfe6c744a493417e7ea1ac1de167 Mon Sep 17 00:00:00 2001 From: dan-searle Date: Fri, 1 Feb 2019 09:45:06 +0000 Subject: [PATCH 11/13] Extract arrayToSentence function to be a component. --- .../x-topic-search/src/ResultContainer.jsx | 17 ++--------------- .../src/SuggestionsAsSentence.jsx | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 15 deletions(-) create mode 100644 components/x-topic-search/src/SuggestionsAsSentence.jsx diff --git a/components/x-topic-search/src/ResultContainer.jsx b/components/x-topic-search/src/ResultContainer.jsx index cc6906168..75ab3dc22 100644 --- a/components/x-topic-search/src/ResultContainer.jsx +++ b/components/x-topic-search/src/ResultContainer.jsx @@ -4,20 +4,7 @@ import classNames from 'classnames'; import SuggestionList from './SuggestionList'; import NoSuggestions from './NoSuggestions'; - -// transform like this => topic1, topic2 and topic3 -const arrayToSentence = followedSuggestions => { - const topicsLength = followedSuggestions.length; - - return followedSuggestions.map((topic, index) => ( - - {topicsLength > 1 && index === topicsLength - 1 && ' and '} - {topic.prefLabel} - {index < topicsLength - 2 && ', '} - - )); -}; - +import SuggestionsAsSentence from './SuggestionsAsSentence'; export default ({ followedSuggestions, searchTerm, csrfToken, followedTopicIds, unfollowedSuggestions }) => { const hasFollowedSuggestions = followedSuggestions.length > 0; @@ -35,7 +22,7 @@ export default ({ followedSuggestions, searchTerm, csrfToken, followedTopicIds, {!hasUnfollowedSuggestions && hasFollowedSuggestions &&
- You already follow { arrayToSentence(followedSuggestions) } + You already follow
} {!hasUnfollowedSuggestions && !hasFollowedSuggestions && diff --git a/components/x-topic-search/src/SuggestionsAsSentence.jsx b/components/x-topic-search/src/SuggestionsAsSentence.jsx new file mode 100644 index 000000000..c7c3f3d8f --- /dev/null +++ b/components/x-topic-search/src/SuggestionsAsSentence.jsx @@ -0,0 +1,17 @@ +import { h } from '@financial-times/x-engine'; + +export default ({ suggestions }) => { + const suggestionsLength = suggestions.length; + + return ( + + {suggestions.map((suggestion, index) => ( + + {suggestionsLength > 1 && index === suggestionsLength - 1 && ' and '} + {suggestion.prefLabel} + {index < suggestionsLength - 2 && ', '} + ) + )} + + ); +}; From b5f80c5660ae6f0383f382cff15e45e1efb9b584 Mon Sep 17 00:00:00 2001 From: dan-searle Date: Fri, 1 Feb 2019 10:56:21 +0000 Subject: [PATCH 12/13] Don't show any search results if the searchTerm length is less than the minSearchLength. --- components/x-topic-search/src/TopicSearch.jsx | 9 +++------ components/x-topic-search/src/lib/get-suggestions.js | 1 - 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/components/x-topic-search/src/TopicSearch.jsx b/components/x-topic-search/src/TopicSearch.jsx index 6cc5ef522..d9a83248d 100644 --- a/components/x-topic-search/src/TopicSearch.jsx +++ b/components/x-topic-search/src/TopicSearch.jsx @@ -46,9 +46,8 @@ class TopicSearch extends Component { if (searchTerm.length >= this.minSearchLength) { this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl, this.props.followedTopicIds) - .then(({ resultsForTerm, followedSuggestions, unfollowedSuggestions }) => { + .then(({ followedSuggestions, unfollowedSuggestions }) => { this.setState({ - resultsForTerm, followedSuggestions, unfollowedSuggestions, showResult: true @@ -56,13 +55,11 @@ class TopicSearch extends Component { }) .catch(() => { this.setState({ - resultsForTerm: null, showResult: false }); }); } else { this.setState({ - resultsForTerm: null, showResult: false }); } @@ -86,7 +83,7 @@ class TopicSearch extends Component { render() { const { csrfToken, followedTopicIds } = this.props; - const { followedSuggestions, resultsForTerm, searchTerm, showResult, unfollowedSuggestions } = this.state; + const { followedSuggestions, searchTerm, showResult, unfollowedSuggestions } = this.state; return (
this.rootEl = el}> @@ -110,7 +107,7 @@ class TopicSearch extends Component { />
- { showResult && resultsForTerm === searchTerm && + { showResult && searchTerm.length >= this.minSearchLength && { return response.json(); }) .then(suggestions => ({ - resultsForTerm: searchTerm, followedSuggestions: suggestions.filter(suggestion => followedTopicIds.includes(suggestion.id)), unfollowedSuggestions: suggestions.filter(suggestion => !followedTopicIds.includes(suggestion.id)) })); From 80c2b45275c3a90cb7ae8ba12cb9e273faa72d6e Mon Sep 17 00:00:00 2001 From: dan-searle Date: Fri, 1 Feb 2019 11:41:32 +0000 Subject: [PATCH 13/13] Remove "you already follow..." messaging and just display any suggestions with their follow buttons in the appropriate states. --- .../__tests__/x-topic-search.test.jsx | 61 +++++-------------- .../x-topic-search/src/ResultContainer.jsx | 32 ---------- .../src/SuggestionsAsSentence.jsx | 17 ------ components/x-topic-search/src/TopicSearch.jsx | 36 +++++------ .../x-topic-search/src/TopicSearch.scss | 8 --- .../x-topic-search/src/lib/get-suggestions.js | 7 +-- 6 files changed, 36 insertions(+), 125 deletions(-) delete mode 100644 components/x-topic-search/src/ResultContainer.jsx delete mode 100644 components/x-topic-search/src/SuggestionsAsSentence.jsx diff --git a/components/x-topic-search/__tests__/x-topic-search.test.jsx b/components/x-topic-search/__tests__/x-topic-search.test.jsx index 5bc356fbc..04fe1b4da 100644 --- a/components/x-topic-search/__tests__/x-topic-search.test.jsx +++ b/components/x-topic-search/__tests__/x-topic-search.test.jsx @@ -3,17 +3,12 @@ const { h } = require('@financial-times/x-engine'); const { mount } = require('@financial-times/x-test-utils/enzyme'); const { TopicSearch } = require('../'); -const searchTerm = 'Dog'; -const searchTermNoResult = 'Blobfish'; -const searchTermAllFollowed = 'Cat'; const minSearchLength = 3; const maxSuggestions = 3; const apiUrl = 'api-url'; -const alreadyFollowedTopics = [ - { uuid: 'Cat-House-id', name: 'Cat House' }, - { uuid: 'Cat-Food-id', name: 'Cat Food' }, - { uuid: 'Cat-Toys-id', name: 'Cat Toys' } -]; +const FOLLOWED_TOPIC_ID1 = 'Cat-House-id'; +const FOLLOWED_TOPIC_ID2 = 'Cat-Food-id'; +const UNFOLLOWED_TOPIC_ID1 = 'Cat-Toys-id'; describe('x-topic-search', () => { const buildSearchUrl = term => `${apiUrl}?count=${maxSuggestions}&partial=${term}`; @@ -29,7 +24,7 @@ describe('x-topic-search', () => { minSearchLength, maxSuggestions, apiUrl, - followedTopicIds: alreadyFollowedTopics.map(topic => topic.uuid), + followedTopicIds: [FOLLOWED_TOPIC_ID1, FOLLOWED_TOPIC_ID2], }; target = mount(); }); @@ -64,17 +59,17 @@ describe('x-topic-search', () => { }); describe('given searchTerm which has some topic suggestions to follow', () => { - const apiUrlWithResults = buildSearchUrl(searchTerm); - const unfollowedTopicSuggestions = [ - { id: 'Dog-House-id', prefLabel: 'Dog House', url: 'Dog-House-url' }, - { id: 'Dog-Food-id', prefLabel: 'Dog Food', url: 'Dog-Food-url' }, - { id: 'Dog-Toys-id', prefLabel: 'Dog Toys', url: 'Dog-Toys-url' } + const apiUrlWithResults = buildSearchUrl('Cat'); + const results = [ + { id: FOLLOWED_TOPIC_ID1, prefLabel: 'Cat House', url: 'Cat-House-url' }, + { id: FOLLOWED_TOPIC_ID2, prefLabel: 'Cat Food', url: 'Cat-Food-url' }, + { id: UNFOLLOWED_TOPIC_ID1, prefLabel: 'Cat Toys', url: 'Cat-Toys-url' } ]; - fetchMock.get(apiUrlWithResults, unfollowedTopicSuggestions); + fetchMock.get(apiUrlWithResults, results); beforeEach(() => { - return enterSearchTerm(searchTerm); + return enterSearchTerm('Cat'); }); it('requests the topic suggestions with count set to maxSuggestions', () => { @@ -89,23 +84,23 @@ describe('x-topic-search', () => { it('renders links and follow buttons for each suggestion', () => { const suggestionsList = target.render().find('li'); - unfollowedTopicSuggestions.forEach((topic, index) => { + results.forEach((topic, index) => { const suggestion = suggestionsList.eq(index); expect(suggestion.find('a').text()).toEqual(topic.prefLabel); expect(suggestion.find('a').attr('href')).toEqual(topic.url); - expect(suggestion.find('button')).toHaveLength(1); + expect(suggestion.find('button').text()).toEqual(topic.id === UNFOLLOWED_TOPIC_ID1 ? 'Add to myFT' : 'Added'); }); }) }); describe('given searchTerm which has no topic suggestions to follow', () => { - const apiUrlNoResults = buildSearchUrl(searchTermNoResult); + const apiUrlNoResults = buildSearchUrl('Dog'); fetchMock.get(apiUrlNoResults, []); beforeEach(() => { - return enterSearchTerm(searchTermNoResult); + return enterSearchTerm('Dog'); }); it('requests from the api and renders the no matching topics message', () => { @@ -117,30 +112,4 @@ describe('x-topic-search', () => { expect(resultContainer.find('h2').text()).toMatch('No topics matching'); }); }); - - describe('given searchTerm which results in all suggestions already followed', () => { - const apiUrlAllFollowed = buildSearchUrl(searchTermAllFollowed); - - fetchMock.get(apiUrlAllFollowed, alreadyFollowedTopics.map(topic => ({ - id: topic.uuid, - prefLabel: topic.name, - url: topic.name.replace(' ', '-') - }))); - - beforeEach(() => { - return enterSearchTerm(searchTermAllFollowed); - }); - - it('requests the suggestions from the api', () => { - expect(fetchMock.called(apiUrlAllFollowed)).toBe(true); - }); - - it('renders the "already followed" message with names of the topics', () => { - const resultContainer = target.render().children('div').eq(1); - - expect(resultContainer).toHaveLength(1); - expect(resultContainer.text()) - .toMatch(`You already follow ${alreadyFollowedTopics[0].name}, ${alreadyFollowedTopics[1].name} and ${alreadyFollowedTopics[2].name}`); - }); - }); }); diff --git a/components/x-topic-search/src/ResultContainer.jsx b/components/x-topic-search/src/ResultContainer.jsx deleted file mode 100644 index 75ab3dc22..000000000 --- a/components/x-topic-search/src/ResultContainer.jsx +++ /dev/null @@ -1,32 +0,0 @@ -import { h } from '@financial-times/x-engine'; -import styles from './TopicSearch.scss'; -import classNames from 'classnames'; - -import SuggestionList from './SuggestionList'; -import NoSuggestions from './NoSuggestions'; -import SuggestionsAsSentence from './SuggestionsAsSentence'; - -export default ({ followedSuggestions, searchTerm, csrfToken, followedTopicIds, unfollowedSuggestions }) => { - const hasFollowedSuggestions = followedSuggestions.length > 0; - const hasUnfollowedSuggestions = unfollowedSuggestions.length > 0; - - return ( -
- {hasUnfollowedSuggestions && - } - - {!hasUnfollowedSuggestions && hasFollowedSuggestions && -
- You already follow -
} - - {!hasUnfollowedSuggestions && !hasFollowedSuggestions && - } -
- ); -}; diff --git a/components/x-topic-search/src/SuggestionsAsSentence.jsx b/components/x-topic-search/src/SuggestionsAsSentence.jsx deleted file mode 100644 index c7c3f3d8f..000000000 --- a/components/x-topic-search/src/SuggestionsAsSentence.jsx +++ /dev/null @@ -1,17 +0,0 @@ -import { h } from '@financial-times/x-engine'; - -export default ({ suggestions }) => { - const suggestionsLength = suggestions.length; - - return ( - - {suggestions.map((suggestion, index) => ( - - {suggestionsLength > 1 && index === suggestionsLength - 1 && ' and '} - {suggestion.prefLabel} - {index < suggestionsLength - 2 && ', '} - ) - )} - - ); -}; diff --git a/components/x-topic-search/src/TopicSearch.jsx b/components/x-topic-search/src/TopicSearch.jsx index d9a83248d..0675f849d 100644 --- a/components/x-topic-search/src/TopicSearch.jsx +++ b/components/x-topic-search/src/TopicSearch.jsx @@ -3,8 +3,8 @@ import styles from './TopicSearch.scss'; import classNames from 'classnames'; import getSuggestions from './lib/get-suggestions.js'; import debounce from 'debounce-promise'; - -import ResultContainer from './ResultContainer'; +import SuggestionList from './SuggestionList'; +import NoSuggestions from './NoSuggestions'; class TopicSearch extends Component { constructor(props) { @@ -20,10 +20,9 @@ class TopicSearch extends Component { this.handleInteractionOutside = this.handleInteractionOutside.bind(this); this.state = { + followedTopicIds: props.followedTopicIds || [], searchTerm: '', - showResult: false, - followedSuggestions: [], - unfollowedSuggestions: [] + showResult: false }; } @@ -45,11 +44,10 @@ class TopicSearch extends Component { this.setState({ searchTerm }); if (searchTerm.length >= this.minSearchLength) { - this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl, this.props.followedTopicIds) - .then(({ followedSuggestions, unfollowedSuggestions }) => { + this.getSuggestions(searchTerm, this.maxSuggestions, this.apiUrl) + .then(({ suggestions }) => { this.setState({ - followedSuggestions, - unfollowedSuggestions, + suggestions, showResult: true }); }) @@ -83,7 +81,7 @@ class TopicSearch extends Component { render() { const { csrfToken, followedTopicIds } = this.props; - const { followedSuggestions, searchTerm, showResult, unfollowedSuggestions } = this.state; + const { searchTerm, showResult, suggestions } = this.state; return (
this.rootEl = el}> @@ -107,13 +105,17 @@ class TopicSearch extends Component { />
- { showResult && searchTerm.length >= this.minSearchLength && - } + {showResult && searchTerm.length >= this.minSearchLength && +
+ {suggestions.length > 0 ? + : + } +
}
); } diff --git a/components/x-topic-search/src/TopicSearch.scss b/components/x-topic-search/src/TopicSearch.scss index bca94f69d..759982612 100644 --- a/components/x-topic-search/src/TopicSearch.scss +++ b/components/x-topic-search/src/TopicSearch.scss @@ -117,11 +117,3 @@ margin-bottom: 0; list-style-type: disc; } - -.all-followed { - @include oTypographySans($scale: 1); - color: oColorsGetPaletteColor('black-70'); - text-align: left; - padding: 0; - margin: 0; -} diff --git a/components/x-topic-search/src/lib/get-suggestions.js b/components/x-topic-search/src/lib/get-suggestions.js index ce2caef4b..d7c85db08 100644 --- a/components/x-topic-search/src/lib/get-suggestions.js +++ b/components/x-topic-search/src/lib/get-suggestions.js @@ -4,7 +4,7 @@ const addQueryParamToUrl = (name, value, url, append = true) => { return append === true ? `${url}&${queryParam}` : `${url}?${queryParam}`; }; -export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { +export default (searchTerm, maxSuggestions, apiUrl) => { const dataSrc = addQueryParamToUrl('count', maxSuggestions, apiUrl, false); const url = addQueryParamToUrl('partial', searchTerm.replace(' ', '+'), dataSrc); @@ -16,8 +16,5 @@ export default (searchTerm, maxSuggestions, apiUrl, followedTopicIds) => { return response.json(); }) - .then(suggestions => ({ - followedSuggestions: suggestions.filter(suggestion => followedTopicIds.includes(suggestion.id)), - unfollowedSuggestions: suggestions.filter(suggestion => !followedTopicIds.includes(suggestion.id)) - })); + .then(suggestions => ({ suggestions })); };