Prep work to sync mobile search in #10424 (#11525)

* Prep work to sync mobile search in #10424

* Added a comment about the custom search event.

* Refactor to fix already defined error caused by const and InstantClick.

* Moved <SearchFormSync /> out of the pack file.

* Fixed broken tests.

* Renamed getInitialSearchTerm utility function to getSearchTermFromUrl

* Added some API docs to getSearchTermFromUrl

* Fixed a typo.

* Added some more API docs.

* Switched to a custom event so a custom payload can be passed.

* Added some API docs.

* Fixed bad destructuring statement.

* wip for some more tests.

* Refactored getSearchTermFromUrl to use URLSearchParams

* lazy loaded state for search term initial value.

* test broken still. Need to think on it.

* filterXSS is now set up in testSetup.js

* Fixed tests.

* Added the @utilities alias to Storybooks webpack config.

* Almost 100% coverage with two useful tests.

* Added some comments explaining document.body usage in the tests.

* Fixed issue where search attemps to get results on first load.

* Small refactor for setting the window.location in tests.

* Clarified an explanation in a comment.
This commit is contained in:
Nick Taylor 2020-11-30 06:13:06 -05:00 committed by GitHub
parent f912f27f34
commit df31a94f2e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 367 additions and 118 deletions

View file

@ -57,6 +57,7 @@ module.exports = {
alias: {
...config.resolve.alias,
'@crayons': path.resolve(__dirname, '../crayons'),
'@utilities': path.resolve(__dirname, '../utilities'),
},
};

View file

@ -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 (
<Fragment>
<KeyboardShortcuts
@ -145,7 +136,7 @@ export class Search extends Component {
this.search(key, value);
}}
onSubmitSearch={this.submit}
searchBoxId={searchBoxId}
ref={this.searchInputRef}
/>
</Fragment>
);
@ -153,5 +144,6 @@ export class Search extends Component {
}
Search.propTypes = {
searchBoxId: PropTypes.string,
searchTerm: PropTypes.string.isRequired,
setSearchTerm: PropTypes.func.isRequired,
};

View file

@ -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,
}) => (
<form
action="/search"
acceptCharset="UTF-8"
method="get"
onSubmit={onSubmitSearch}
>
<input name="utf8" type="hidden" value="✓" />
<input
className="crayons-header--search-input crayons-textfield"
type="text"
name="q"
id={searchBoxId}
placeholder="Search..."
autoComplete="off"
aria-label="search"
onKeyDown={onSearch}
value={searchTerm}
/>
</form>
export const SearchForm = forwardRef(
({ searchTerm, onSearch, onSubmitSearch }, ref) => (
<form
action="/search"
acceptCharset="UTF-8"
method="get"
onSubmit={onSubmitSearch}
>
<input name="utf8" type="hidden" value="✓" />
<input
ref={ref}
className="crayons-header--search-input crayons-textfield"
type="text"
name="q"
placeholder="Search..."
autoComplete="off"
aria-label="search"
onKeyDown={onSearch}
value={searchTerm}
/>
</form>
),
);
SearchForm.propTypes = {
searchTerm: PropTypes.string.isRequired,
searchBoxId: PropTypes.string.isRequired,
onSearch: PropTypes.func.isRequired,
onSubmitSearch: PropTypes.func.isRequired,
};

View file

@ -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 (
<Fragment>
<Search searchTerm={searchTerm} setSearchTerm={setSearchTerm} />
{mobileSearchContainer &&
createPortal(
<Search searchTerm={searchTerm} setSearchTerm={setSearchTerm} />,
mobileSearchContainer,
)}
</Fragment>
);
}

View file

@ -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();

View file

@ -17,7 +17,11 @@ describe('<Search />', () => {
});
it('should have no a11y violations', async () => {
const { container } = render(<Search />);
const props = {
searchTerm: 'fish',
setSearchTerm: jest.fn(),
};
const { container } = render(<Search {...props} />);
const results = await axe(container);
@ -25,20 +29,30 @@ describe('<Search />', () => {
});
it('should have a search textbox', () => {
const { getByLabelText } = render(<Search />);
const props = {
searchTerm: 'fish',
setSearchTerm: jest.fn(),
};
const { getByLabelText } = render(<Search {...props} />);
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(<Search />);
const props = {
searchTerm: 'fish',
setSearchTerm: jest.fn(),
};
const { getByLabelText, findByLabelText } = render(<Search {...props} />);
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('<Search />', () => {
expect(searchInput.value).toEqual('hello');
});
it('should set the search term', async () => {
const props = {
searchTerm: '',
setSearchTerm: jest.fn(),
};
const { getByLabelText } = render(<Search {...props} />);
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(<Search />);
const props = {
searchTerm: '',
setSearchTerm: jest.fn(),
};
const { getByLabelText, findByLabelText } = render(<Search {...props} />);
let searchInput = getByLabelText(/search/i);
@ -85,7 +128,11 @@ describe('<Search />', () => {
// listener is registered as it affects the UI.
jest.spyOn(window, 'addEventListener');
render(<Search />);
const props = {
searchTerm: '',
setSearchTerm: jest.fn(),
};
render(<Search {...props} />);
expect(window.addEventListener).toHaveBeenNthCalledWith(
1,
@ -99,7 +146,11 @@ describe('<Search />', () => {
// listener is unregistered as it affects the UI.
jest.spyOn(window, 'removeEventListener');
const { unmount } = render(<Search />);
const props = {
searchTerm: '',
setSearchTerm: jest.fn(),
};
const { unmount } = render(<Search {...props} />);
unmount();

View file

@ -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('<SearchFormSync />', () => {
// 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(<SearchFormSync />, {
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 =
'<div id="mobile-search-container"><form></form></div>';
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(<SearchFormSync />, {
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 =
'<div id="mobile-search-container"><form></form></div>';
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 =
'<div id="mobile-search-container"><form></form></div>';
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);
});
});

View file

@ -1,2 +1,3 @@
export * from './Search';
export * from './SearchForm';
export * from './SearchFormSync';

View file

@ -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(<Search />, root);
render(<SearchFormSync />, root);
});

View file

@ -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 = `<script>alert('XSS!');</script>`;
const querystring = `?q=<script>alert(%27XSS!%27);</script>`;
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');
});
});
});

View file

@ -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.
*

View file

@ -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 } }));
</script>