diff --git a/app/javascript/.storybook/main.js b/app/javascript/.storybook/main.js index 5ee7e32f7..eb52b8117 100644 --- a/app/javascript/.storybook/main.js +++ b/app/javascript/.storybook/main.js @@ -57,6 +57,7 @@ module.exports = { alias: { ...config.resolve.alias, '@crayons': path.resolve(__dirname, '../crayons'), + '@utilities': path.resolve(__dirname, '../utilities'), }, }; diff --git a/app/javascript/Search/Search.jsx b/app/javascript/Search/Search.jsx index b3656b5c5..370c7e98e 100644 --- a/app/javascript/Search/Search.jsx +++ b/app/javascript/Search/Search.jsx @@ -1,10 +1,10 @@ -import { h, Component, Fragment } from 'preact'; +import { h, Component, Fragment, createRef } from 'preact'; import PropTypes from 'prop-types'; import { - getInitialSearchTerm, + displaySearchResults, + getSearchTermFromUrl, hasInstantClick, preloadSearchResults, - displaySearchResults, } from '../utilities/search'; import { KeyboardShortcuts } from '../shared/components/useKeyboardShortcuts'; import { SearchForm } from './SearchForm'; @@ -14,33 +14,23 @@ const GLOBAL_SEARCH_KEY = 'Slash'; const ENTER_KEY = 'Enter'; export class Search extends Component { - static defaultProps = { - searchBoxId: 'nav-search', - }; - constructor(props) { super(props); this.enableSearchPageChecker = true; this.syncSearchUrlWithInput = this.syncSearchUrlWithInput.bind(this); + this.searchInputRef = createRef(null); } componentWillMount() { - let searchTerm; + const { searchTerm, setSearchTerm } = this.props; - ({ searchTerm } = this.state); - this.setState( - { searchTerm: getInitialSearchTerm(window.location.search) }, - () => preloadSearchResults({ searchTerm }), - ); - - ({ searchTerm } = this.state); const searchPageChecker = () => { if ( this.enableSearchPageChecker && searchTerm !== '' && /^http(s)?:\/\/[^/]+\/search/.exec(window.location.href) === null ) { - this.setState({ searchTerm: '' }); + setSearchTerm(''); } setTimeout(searchPageChecker, 500); @@ -56,19 +46,17 @@ export class Search extends Component { // TODO: Consolidate search functionality. // Note that push states for search occur in _search.html.erb // in initializeSortingTabs(query) - const { searchBoxId } = this.props; - const searchTerm = getInitialSearchTerm(window.location.search); + const { setSearchTerm } = this.props; + const searchTerm = getSearchTermFromUrl(window.location.search); // We set the value outside of React state so that there is no flickering of placeholder // to search term. - const searchBox = document.getElementById(searchBoxId); + const searchBox = this.searchInputRef.current; searchBox.value = searchTerm; // Even though we set the search term directly via the DOM, it still needs to reside // in component state. - this.setState({ - searchTerm, - }); + setSearchTerm(searchTerm); } componentDidMount() { @@ -88,27 +76,31 @@ export class Search extends Component { submit = (event) => { if (hasInstantClick) { event.preventDefault(); - - const { searchTerm } = this.state; - displaySearchResults({ searchTerm }); } }; - search(key, value) { + search(key, searchTerm) { + const { searchTerm: currentSearchTerm } = this.props; this.enableSearchPageChecker = false; - if (hasInstantClick() && key === ENTER_KEY) { - this.setState({ searchTerm: value }, () => { - const { searchTerm } = this.state; - preloadSearchResults({ searchTerm }); - }); + if ( + hasInstantClick() && + key === ENTER_KEY && + currentSearchTerm !== searchTerm + ) { + const { setSearchTerm } = this.props; + + setSearchTerm(searchTerm); + preloadSearchResults({ searchTerm }); + displaySearchResults({ searchTerm }); } } componentWillUnmount() { document.removeEventListener('keydown', this.globalKeysListener); window.removeEventListener('popstate', this.syncSearchUrlWithInput); - InstantClick.off('change', this.enableSearchPageListener); + InstantClick.off && + InstantClick.off('change', this.enableSearchPageListener); } minimizeHeader = (event) => { @@ -120,13 +112,12 @@ export class Search extends Component { event.preventDefault(); document.body.classList.remove('zen-mode'); - const { searchBoxId } = this.props; - const searchBox = document.getElementById(searchBoxId); + const searchBox = this.searchInputRef.current; searchBox.focus(); searchBox.select(); }; - render({ searchBoxId }, { searchTerm = '' }) { + render({ searchTerm }) { return ( ); @@ -153,5 +144,6 @@ export class Search extends Component { } Search.propTypes = { - searchBoxId: PropTypes.string, + searchTerm: PropTypes.string.isRequired, + setSearchTerm: PropTypes.func.isRequired, }; diff --git a/app/javascript/Search/SearchForm.jsx b/app/javascript/Search/SearchForm.jsx index 4db230205..796bf8234 100644 --- a/app/javascript/Search/SearchForm.jsx +++ b/app/javascript/Search/SearchForm.jsx @@ -1,36 +1,33 @@ import PropTypes from 'prop-types'; import { h } from 'preact'; +import { forwardRef } from 'preact/compat'; -export const SearchForm = ({ - searchTerm, - onSearch, - onSubmitSearch, - searchBoxId, -}) => ( -
- - -
+export const SearchForm = forwardRef( + ({ searchTerm, onSearch, onSubmitSearch }, ref) => ( +
+ + +
+ ), ); SearchForm.propTypes = { searchTerm: PropTypes.string.isRequired, - searchBoxId: PropTypes.string.isRequired, onSearch: PropTypes.func.isRequired, onSubmitSearch: PropTypes.func.isRequired, }; diff --git a/app/javascript/Search/SearchFormSync.jsx b/app/javascript/Search/SearchFormSync.jsx new file mode 100644 index 000000000..1e95d0fd5 --- /dev/null +++ b/app/javascript/Search/SearchFormSync.jsx @@ -0,0 +1,68 @@ +import { h } from 'preact'; +import { useEffect, useState } from 'preact/hooks'; +import { createPortal, Fragment, unmountComponentAtNode } from 'preact/compat'; +import { Search } from './Search'; +import { getSearchTermFromUrl } from '@utilities/search'; + +/** + * Manages the synchronization of search state between the top search bar (desktop) and + * mobile (in search results page). + */ +export function SearchFormSync() { + const [searchTerm, setSearchTerm] = useState(() => { + return getSearchTermFromUrl(location.search); + }); + const [mobileSearchContainer, setMobileSearchContainer] = useState(null); + + /** + * A listener for handling the synchronization of search forms. + * + * @param {CustomEvent<{ querystring: string }>} event A custom event for synching search forms. + */ + function syncSearchFormsListener(event) { + const { querystring } = event.detail; + const updatedSearchTerm = getSearchTermFromUrl(querystring); + + // Server-side rendering of search results means the DOM node is destroyed everytime a search is performed, + // So we need to get the reference every time and use that for the parent in the portal. + const element = document.getElementById('mobile-search-container'); + + // The DOM element has changed because server-sde rendering returns new + // search results which destroys the existing search form in mobile view. + // Because of this we need to unmount the component at the old element reference + // i.e. the container for the createPortal call in the render. + // If we do not unmount, it will result in an unmounting error that will throw as the + // container element (search form that was wiped out because of the new search results) no longer exists. + if (mobileSearchContainer && element !== mobileSearchContainer) { + unmountComponentAtNode(mobileSearchContainer); + } + + // We need to delete the existing server-side rendered form because createPortal only appends to it's container. + if (element) { + const form = element.querySelector('form'); + form && element.removeChild(form); + } + + setMobileSearchContainer(element); + setSearchTerm(updatedSearchTerm); + } + + useEffect(() => { + window.addEventListener('syncSearchForms', syncSearchFormsListener); + + return () => { + window.removeEventListener('syncSearchForms', syncSearchFormsListener); + }; + }); + + return ( + + + {mobileSearchContainer && + createPortal( + , + mobileSearchContainer, + )} + + ); +} diff --git a/app/javascript/Search/__stories__/SearchForm.stories.jsx b/app/javascript/Search/__stories__/SearchForm.stories.jsx index 27946f9fe..8afce75f9 100644 --- a/app/javascript/Search/__stories__/SearchForm.stories.jsx +++ b/app/javascript/Search/__stories__/SearchForm.stories.jsx @@ -4,7 +4,6 @@ import { action } from '@storybook/addon-actions'; import { SearchForm } from '..'; const commonProps = { - searchBoxId: 'nav-search', onSearch: action('on preloading search'), onSubmitSearch: (e) => { e.preventDefault(); diff --git a/app/javascript/Search/__tests__/Search.test.jsx b/app/javascript/Search/__tests__/Search.test.jsx index 4568dd2cb..e3a3e0a0c 100644 --- a/app/javascript/Search/__tests__/Search.test.jsx +++ b/app/javascript/Search/__tests__/Search.test.jsx @@ -17,7 +17,11 @@ describe('', () => { }); it('should have no a11y violations', async () => { - const { container } = render(); + const props = { + searchTerm: 'fish', + setSearchTerm: jest.fn(), + }; + const { container } = render(); const results = await axe(container); @@ -25,20 +29,30 @@ describe('', () => { }); it('should have a search textbox', () => { - const { getByLabelText } = render(); + const props = { + searchTerm: 'fish', + setSearchTerm: jest.fn(), + }; + + const { getByLabelText } = render(); const searchInput = getByLabelText(/search/i); + expect(searchInput.value).toEqual('fish'); expect(searchInput.getAttribute('placeholder')).toEqual('Search...'); expect(searchInput.getAttribute('autocomplete')).toEqual('off'); }); it('should contain text the user entered in the search textbox', async () => { - const { getByLabelText, findByLabelText } = render(); + const props = { + searchTerm: 'fish', + setSearchTerm: jest.fn(), + }; + const { getByLabelText, findByLabelText } = render(); let searchInput = getByLabelText(/search/i); - expect(searchInput.value).toEqual(''); + expect(searchInput.value).toEqual('fish'); // user.type doesn't work in the case of // search as the current implementation is relying on keydown @@ -55,10 +69,39 @@ describe('', () => { expect(searchInput.value).toEqual('hello'); }); + it('should set the search term', async () => { + const props = { + searchTerm: '', + setSearchTerm: jest.fn(), + }; + const { getByLabelText } = render(); + + let searchInput = getByLabelText(/search/i); + + expect(searchInput.value).toEqual(''); + + // user.type doesn't work in the case of + // search as the current implementation is relying on keydown + // events + fireEvent.keyDown(searchInput, { + key: 'Enter', + keyCode: 13, + which: 13, + target: { value: 'hello' }, + }); + + expect(searchInput.value).toEqual('hello'); + expect(props.setSearchTerm).toHaveBeenCalledWith('hello'); + }); + it('should submit the search form', async () => { jest.spyOn(Search.prototype, 'search'); - const { getByLabelText, findByLabelText } = render(); + const props = { + searchTerm: '', + setSearchTerm: jest.fn(), + }; + const { getByLabelText, findByLabelText } = render(); let searchInput = getByLabelText(/search/i); @@ -85,7 +128,11 @@ describe('', () => { // listener is registered as it affects the UI. jest.spyOn(window, 'addEventListener'); - render(); + const props = { + searchTerm: '', + setSearchTerm: jest.fn(), + }; + render(); expect(window.addEventListener).toHaveBeenNthCalledWith( 1, @@ -99,7 +146,11 @@ describe('', () => { // listener is unregistered as it affects the UI. jest.spyOn(window, 'removeEventListener'); - const { unmount } = render(); + const props = { + searchTerm: '', + setSearchTerm: jest.fn(), + }; + const { unmount } = render(); unmount(); diff --git a/app/javascript/Search/__tests__/SearchFormSync.test.jsx b/app/javascript/Search/__tests__/SearchFormSync.test.jsx new file mode 100644 index 000000000..bb8a7ded3 --- /dev/null +++ b/app/javascript/Search/__tests__/SearchFormSync.test.jsx @@ -0,0 +1,126 @@ +import { h } from 'preact'; +import { fireEvent, render } from '@testing-library/preact'; +import { SearchFormSync } from '../SearchFormSync'; + +function setWindowLocation(url = '') { + delete window.location; // Inspired from https://www.benmvp.com/blog/mocking-window-location-methods-jest-jsdom/ + window.location = new URL(url); +} + +// a11y tests are not required for this component as it's job is to provide data to other components. +// There is nothing UI related about this component. +describe('', () => { + // For some reason when document.body is used for renders, we need to clear out the rendered markup in it. + // My guess is that Preact testing library handles this internally when using the default container to render in. + + beforeEach(() => { + setWindowLocation(`https://locahost:3000/`); + + // The body is being cleared out because we are using it as the root element for the tests. + // Typically using the document.body as the root for rendering of components in tests is not necessary, + // but in the case of this component, it renders a portal, and this seemed to be the only way to get these + // tests to render portals. + document.body.innerHTML = ''; + + global.InstantClick = jest.fn(() => ({ + on: jest.fn(), + off: jest.fn(), + preload: jest.fn(), + display: jest.fn(), + }))(); + }); + + it('should synchronize search forms', async () => { + const { findByLabelText, findAllByLabelText } = render(, { + container: document.body, + }); + + // Only one input is rendered at this point because the synchSearchForms custom event is what + // tells us that there is a new search form to sync with the existing one. + const searchInput = await findByLabelText('search'); + + // Because window.location has no search term in it's URL + expect(searchInput.value).toEqual(''); + + // https://www.theatlantic.com/technology/archive/2012/09/here-it-is-the-best-word-ever/262348/ + const searchTerm = 'diphthong'; + + // simulates a search result returned which contains the server side rendered search form for mobile only. + setWindowLocation(`https://locahost:3000/search?q=${searchTerm}`); + + // This part of the DOM would be rendered in the search results from the server side. + // See search.html.erb. + document.body.innerHTML = + '
'; + + fireEvent( + window, + new CustomEvent('syncSearchForms', { + detail: { querystring: window.location.search }, + }), + ); + + const [desktopSearch, mobileSearch] = await findAllByLabelText('search'); + + expect(desktopSearch.value).toEqual(searchTerm); + expect(mobileSearch.value).toEqual(searchTerm); + }); + + it('should synchronize search forms on a subsequent search', async () => { + const { findByLabelText, findAllByLabelText } = render(, { + container: document.body, + }); + + // Only one input is rendered at this point because the synchSearchForms custom event is what + // tells us that there is a new search form to sync with the existing one. + const searchInput = await findByLabelText('search'); + + // Because window.location has no search term in it's URL + expect(searchInput.value).toEqual(''); + + // https://www.theatlantic.com/technology/archive/2012/09/here-it-is-the-best-word-ever/262348/ + const searchTerm = 'diphthong'; + + // simulates a search result returned which contains the server side rendered search form for mobile only. + setWindowLocation(`https://locahost:3000/search?q=${searchTerm}`); + + // This part of the DOM would be rendered in the search results from the server side. + // See search.html.erb. + document.body.innerHTML = + '
'; + + fireEvent( + window, + new CustomEvent('syncSearchForms', { + detail: { querystring: window.location.search }, + }), + ); + + let [desktopSearch, mobileSearch] = await findAllByLabelText('search'); + + expect(desktopSearch.value).toEqual(searchTerm); + expect(mobileSearch.value).toEqual(searchTerm); + + const searchTerm2 = 'diphthong2'; + + // simulates a search result returned which contains the server side rendered search form for mobile only. + setWindowLocation(`https://locahost:3000/search?q=${searchTerm2}`); + + // This part of the DOM would be rendered in the search results from the server side. + // See search.html.erb. + document.body.innerHTML = + '
'; + + fireEvent( + window, + new CustomEvent('syncSearchForms', { + detail: { querystring: window.location.search }, + }), + ); + + [desktopSearch, mobileSearch] = await findAllByLabelText('search'); + + expect(desktopSearch.value).toEqual(searchTerm2); + expect(mobileSearch.value).toEqual(searchTerm2); + }); +}); diff --git a/app/javascript/Search/index.js b/app/javascript/Search/index.js index f5121db3b..41ee9f98e 100644 --- a/app/javascript/Search/index.js +++ b/app/javascript/Search/index.js @@ -1,2 +1,3 @@ export * from './Search'; export * from './SearchForm'; +export * from './SearchFormSync'; diff --git a/app/javascript/packs/Search.jsx b/app/javascript/packs/Search.jsx index 188c586be..54fb88a5f 100644 --- a/app/javascript/packs/Search.jsx +++ b/app/javascript/packs/Search.jsx @@ -1,9 +1,9 @@ import { h, render } from 'preact'; -import { Search } from '../Search'; import 'focus-visible'; +import { SearchFormSync } from '../Search/SearchFormSync'; document.addEventListener('DOMContentLoaded', () => { const root = document.getElementById('header-search'); - render(, root); + render(, root); }); diff --git a/app/javascript/utilities/__tests__/search.test.js b/app/javascript/utilities/__tests__/search.test.js index 604518b29..69704d3d4 100644 --- a/app/javascript/utilities/__tests__/search.test.js +++ b/app/javascript/utilities/__tests__/search.test.js @@ -1,11 +1,10 @@ import fetch from 'jest-fetch-mock'; import { - getInitialSearchTerm, + getSearchTermFromUrl, preloadSearchResults, hasInstantClick, displaySearchResults, fetchSearch, - createSearchUrl, } from '../search'; import '../../../assets/javascripts/lib/xss'; @@ -23,12 +22,12 @@ describe('Search utilities', () => { delete globalThis.getCsrfToken; }); - describe('getInitialSearchTerm', () => { + describe('getSearchTermFromUrl', () => { describe(`When the querystring key 'q' has a value`, () => { it(`should return the querystring key q's value`, () => { const expected = 'hello'; const querystring = `?q=${expected}`; - const actual = getInitialSearchTerm(querystring); + const actual = getSearchTermFromUrl(querystring); expect(actual).toEqual(expected); }); }); @@ -37,7 +36,7 @@ describe('Search utilities', () => { it(`should return the querystring key q's decoded value with + characters replaced by a space`, () => { const expected = `my visual studio setup`; const querystring = `?q=my+visual+studio+setup`; - const actual = getInitialSearchTerm(querystring); + const actual = getSearchTermFromUrl(querystring); expect(actual).toEqual(expected); }); }); @@ -46,7 +45,7 @@ describe('Search utilities', () => { it(`should return the querystring key q's decoded value`, () => { const expected = ``; const querystring = `?q=`; - const actual = getInitialSearchTerm(querystring); + const actual = getSearchTermFromUrl(querystring); expect(actual).toEqual(expected); }); }); @@ -55,7 +54,7 @@ describe('Search utilities', () => { it(`should return an empty string`, () => { const expected = ''; const querystring = `?q=`; - const actual = getInitialSearchTerm(querystring); + const actual = getSearchTermFromUrl(querystring); expect(actual).toEqual(expected); }); @@ -64,7 +63,7 @@ describe('Search utilities', () => { filterXSS = jest.fn(() => undefined); const querystring = `?q=`; - const actual = getInitialSearchTerm(querystring); + const actual = getSearchTermFromUrl(querystring); expect(actual).toEqual(''); }); }); @@ -73,7 +72,7 @@ describe('Search utilities', () => { it(`should return an empty string`, () => { const expected = ''; const querystring = '?'; - const actual = getInitialSearchTerm(querystring); + const actual = getSearchTermFromUrl(querystring); expect(actual).toEqual(expected); }); }); @@ -259,13 +258,4 @@ describe('Search utilities', () => { expect(response).toMatchObject(expected); }); }); - - describe('createSearchUrl', () => { - it('should return a url string', () => { - const dataHash = { name: 'jav', tags: ['one', 'two'] }; - const responseString = createSearchUrl(dataHash); - - expect(responseString).toEqual('name=jav&tags%5B%5D=one&tags%5B%5D=two'); - }); - }); }); diff --git a/app/javascript/utilities/search/index.js b/app/javascript/utilities/search/index.js index ff873141e..e72c9af53 100644 --- a/app/javascript/utilities/search/index.js +++ b/app/javascript/utilities/search/index.js @@ -1,7 +1,7 @@ // TODO: We should really be using the xss package by installing it in package.json // but for now filterXSS is global because of legacy JS -import { request } from '../http'; +import { request } from '@utilities/http'; function getParameterByName(name, url = window.location.href) { const sanitizedName = name.replace(/[[\]]/g, '\\$&'); @@ -40,16 +40,43 @@ function getSortParameters(url) { return sortBy + sortDirection; } -export const hasInstantClick = () => typeof instantClick !== 'undefined'; +/** + * Determines whether or not InstantClick is enabled. + * + * @returns True if InstantClick is enabled, otherwise false. + */ +export function hasInstantClick() { + return typeof instantClick !== 'undefined'; +} -function fixedEncodeURIComponent(str) { +function fixedEncodeURIComponent(value) { // from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent - return encodeURIComponent(str).replace( + return encodeURIComponent(value).replace( /[!'()*]/g, (c) => `%${c.charCodeAt(0).toString(16)}`, ); } +function createSearchUrl(dataHash) { + const searchParams = new URLSearchParams(); + Object.keys(dataHash).forEach((key) => { + const value = dataHash[key]; + if (Array.isArray(value)) { + value.forEach((arrayValue) => { + searchParams.append(`${key}[]`, arrayValue); + }); + } else { + searchParams.append(key, value); + } + }); + + return searchParams.toString(); +} + +/** + * + * @param {*} param0 + */ export function displaySearchResults({ searchTerm, location = window.location, @@ -64,14 +91,18 @@ export function displaySearchResults({ ); } -export function getInitialSearchTerm(querystring) { - const matches = /(?:&|\?)?q=([^&=]+)/.exec(querystring); - const rawSearchTerm = - matches !== null && matches.length === 2 - ? decodeURIComponent(matches[1].replace(/\+/g, '%20')) - : ''; - const query = filterXSS(rawSearchTerm) || ''; +/** + * Extracts the search term from an URL's query string. + * + * @param {string} querystring A URL query string + * + * @returns The extracted search term from the query string + */ +export function getSearchTermFromUrl(querystring) { + const searchParameters = new URLSearchParams(querystring); + const query = filterXSS(searchParameters.get('q')) ?? ''; const divForDecode = document.createElement('div'); + divForDecode.innerHTML = query; return divForDecode.firstChild !== null @@ -79,6 +110,12 @@ export function getInitialSearchTerm(querystring) { : query; } +/** + * Preloads search results for the given search term + * @param {string} searchTerm The search term + * @param {Location} location[window.location] The location (URL) of the object it is linked to. + * By default it is linked to the Window object. + */ export function preloadSearchResults({ searchTerm, location = window.location, @@ -92,22 +129,6 @@ export function preloadSearchResults({ InstantClick.preload(searchUrl); } -export function createSearchUrl(dataHash) { - const searchParams = new URLSearchParams(); - Object.keys(dataHash).forEach((key) => { - const value = dataHash[key]; - if (Array.isArray(value)) { - value.forEach((arrayValue) => { - searchParams.append(`${key}[]`, arrayValue); - }); - } else { - searchParams.append(key, value); - } - }); - - return searchParams.toString(); -} - /** * A helper method to call /search endpoints. * diff --git a/app/views/stories/_stories_list_script.html.erb b/app/views/stories/_stories_list_script.html.erb index 606f0fa6a..c0f0576c1 100644 --- a/app/views/stories/_stories_list_script.html.erb +++ b/app/views/stories/_stories_list_script.html.erb @@ -23,4 +23,7 @@ } }); } + + // A custom event that gets dispatched to notify search forms to synchronize their state. + window.dispatchEvent(new CustomEvent('syncSearchForms', { detail: { querystring: location.search } }));