feat(Highlight): support array of strings #715

marielaures · 2017-12-12T15:47:50Z

Summary

This PR fixes #22.

Before this, the Highlight widget was only compatible with strings.
Now we've added support for arrays of strings.

Result

The Highlight widget is used in the same way for attribute names that point to either a string or an array of strings.

In the following example we have two Highlight widgets, the first one for attributeName="name" where hit.name is a string, and the second one for attributeName="tags" where hit.tags is an array of strings (tags: ["Ancient Egypt", "Online Course", "History"] ):

<Highlight attributeName="name" hit={hit} />
<Highlight attributeName="tags" hit={hit} />

algobot · 2017-12-12T15:55:13Z

Deploy preview ready!

Built with commit 268a5ac

https://deploy-preview-715--react-instantsearch.netlify.com

Haroenv · 2017-12-12T16:38:19Z

docgen/plugins/rev-assets.js

-  string
->): Promise<*> {
+function computeFiles(
+  [filesGlob, replaceFilesGlob]: Array<string, string>


seems like a different version of prettier was run over the code base

Haroenv · 2017-12-12T16:38:48Z

packages/react-instantsearch/src/components/Highlighter.js

+Highlighter.defaultProps = {
+  tagName: 'em',
+  nonHighlightedTagName: 'span',
+  separator: ',',


default should be ,<space>

samouss

Nice work @marielaures! 🙂

I left few comments, but no blockers for me.

samouss · 2017-12-12T18:43:37Z

packages/react-instantsearch/src/components/Highlighter.js

@@ -1,35 +1,75 @@
 import PropTypes from 'prop-types';
 import React from 'react';
+import classNames from './classNames.js';


We don't need the .js at the end of the import.

samouss · 2017-12-12T18:44:51Z

packages/react-instantsearch/src/components/Highlight.js

@@ -11,4 +11,6 @@ Highlight.propTypes = {
  attributeName: PropTypes.string.isRequired,
  highlight: PropTypes.func.isRequired,
  tagName: PropTypes.string,
+  nonHighlightedTagName: PropTypes.string,
+  separator: PropTypes.string,


It should match the PropTypes on Highlighter(aka: node). We also should rename separator for separatorComponent. For example in SearchBox all custom components are prefix with Component (ex: ResetComponent, SubmitComponent). And finally we need to replicate the props on the Snippet component.

samouss · 2017-12-12T18:50:16Z

packages/react-instantsearch/src/components/Highlighter.js

+            <span key={`split-${i}-${hit[attributeName][i]}`}>
+              {item.map((element, index) => (
+                <Highlight
+                  key={`split-${index}-${element.value}`}


We can extract the key generation in a function since we use it tree times.

The key generation is slightly different, since in one case we're accessing an element of an array (hit[attributeName][i]) and for the case where attributeName is a string, we do element.value. Do I do a function a maybe pass an attribute which will be a Boolean to specify whether it is an array and return either the first option or the second or leave it as is?

Nope, you can simply create function that takes (index, value) as parameters. Then you called it with the according values (ex: element.value, hit[attributeName][i]).

samouss · 2017-12-12T18:55:55Z

packages/react-instantsearch/src/core/highlight.js

@@ -14,7 +14,7 @@ import { get } from 'lodash';
 * @param {string} highlightProperty - the property that contains the highlight structure in the results
 * @param {string} attributeName - the highlighted attribute to look for
 * @param {object} hit - the actual hit returned by Algolia.
- * @return {object[]} - An array of {value: string, isDefined: boolean}.
+ * @return {object[]} - An array of {value: string,  isDefined: boolean}.


We can update the documentation, it's isHighlighted instead of isDefined.

samouss · 2017-12-12T18:57:29Z

packages/react-instantsearch/src/core/highlight.js

+          postTag,
+          highlightedValue: item.value,
+        })
+      );


The ternary complicate a bit the comprehension of the function. We can revert back to a simpler if statement.

vvo · 2017-12-14T12:17:36Z

packages/react-instantsearch/src/components/Highlighter.js

+  return `split-${i}-${value}`;
+}
+
+export const Highlight = ({


Should we export this as a component and then test the rendering out of it? So we freeze the implementation. Right now we are only testing that Highlight is called with some props, not the actual rendering of it (because it's shallow rendered in Highlighter.test.js)

What do you mean by freezing the implementation? Highlight is tested separately so we don't need to do a full DOM rendering in Highlighter. We can just test that the the correct props are applied on the component. I think it's easier to reason about unit, rather than the all implementation.

What I meant is that right no there's no test ensuring that the output of this single Highlight component is some specific DOM.

Which means that if we change the output without even touching the props being passed, nothing will break, while the public API contract will break (DOM will change).

If that's not clear still let me know

Nope perfectly clear! But we already test the output of the Highlight component.

react-instantsearch/packages/react-instantsearch/src/components/Highlighter.test.js

Lines 11 to 39 in f3522ad

describe('Highlighter - Highlight', () => {

const defaultProps = {

value: 'test',

highlightedTagName: 'em',

isHighlighted: false,

nonHighlightedTagName: 'div',

};

it('renders a highlight', () => {

const props = {

...defaultProps,

isHighlighted: true,

};

const wrapper = shallow(<Highlight {...props} />);

expect(wrapper).toMatchSnapshot();

});

it('renders a nonhighlight', () => {

const props = {

...defaultProps,

};

const wrapper = shallow(<Highlight {...props} />);

expect(wrapper).toMatchSnapshot();

});

});

vvo · 2017-12-17T23:59:01Z

packages/react-instantsearch/src/components/Highlighter.js

+  return `split-${i}-${value}`;
+}
+
+export const Highlight = ({


samouss

One last thing, we can update the connectHighlight documentation. First we can add attributeName, hit, highlightProperty to the section "Exposed Props". They are required since the provided prop highlight doesn't work without them. We can also rewrite the example because it's a bit complicated to understand how it works with the current code snippet.

Last, we can also precise that the highlight prop can now return a nested array depending on the shape of the highlighted property.

<a name="4.3.0"></a> * reset page with multi index ([#665](#665)) ([865b7dc](865b7dc)) * **SearchBox:** use hidden over style to hide loader ([#714](#714)) ([ed5eb77](ed5eb77)) * test recipes ([#740](#740)) ([de2cc37](de2cc37)) * track all index in the manager ([#660](#660)) ([793502b](793502b)) * loading indicator ([#544](#544)) ([189659e](189659e)) * **Highlight:** support array of strings ([#715](#715)) ([8e93c6a](8e93c6a))

Haroenv reviewed Dec 12, 2017

View reviewed changes

marielaures force-pushed the feat/highlightarray branch from e0fd5c4 to 8d838d2 Compare December 12, 2017 17:37

samouss reviewed Dec 12, 2017

View reviewed changes

marielaures added 17 commits December 13, 2017 12:37

feat(highlightArray): plug new index to a story

e1fc74d

feat(highlightArray): add test for highlight

2cdd95f

feat(highlightArray): visually highlight the element in arrays

39ed72e

feat(highlightArray): wrap separator with span

dc4d702

feat(highlightArray): modify story

7400d67

feat(highlightArray): move tagName logic to Highlighter component

b10dc35

feat(highlightArray): manage className with cx

f7fbd53

feat(highlightArray): change className to render style

40af515

feat(highlightArray): move back to tagName

5450847

feat(highlightArray): test Highlighter

e56c1f4

feat(highlightArray): merge two stories in Storybook

b2e64b0

feat(highlightArray): updating Highlighting results guide

1925190

feat(highlightArray): update Highlight and Snippet Widgets documentation

6d451bc

feat(highlightArray): fix linting

8b39e72

feat(highlightArray): fix linting error

de26226

feat(highlightArray): updates to address reviews on PR

e07dda4

feat(highlightArray): add function to generate key for iterables

ae8418a

marielaures force-pushed the feat/highlightarray branch from 8fd36cc to ae8418a Compare December 13, 2017 11:37

vvo suggested changes Dec 14, 2017

View reviewed changes

vvo approved these changes Dec 17, 2017

View reviewed changes

packages/react-instantsearch/src/components/Highlighter.js

return `split-${i}-${value}`;

}

export const Highlight = ({

Copy link

Contributor

vvo Dec 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

samouss suggested changes Dec 18, 2017

View reviewed changes

feat(highlightArray): update documentation after feedback on PR

c8be162

marielaures force-pushed the feat/highlightarray branch from 7f7bb50 to c8be162 Compare December 18, 2017 12:42

Merge branch 'master' into feat/highlightarray

268a5ac

samouss approved these changes Dec 18, 2017

View reviewed changes

samouss merged commit 8e93c6a into master Dec 19, 2017

samouss deleted the feat/highlightarray branch December 19, 2017 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Highlight): support array of strings #715

feat(Highlight): support array of strings #715

marielaures commented Dec 12, 2017 •

edited by samouss

Loading

algobot commented Dec 12, 2017 •

edited

Loading

Haroenv Dec 12, 2017

Haroenv Dec 12, 2017 •

edited

Loading

samouss left a comment

samouss Dec 12, 2017

samouss Dec 12, 2017

samouss Dec 12, 2017

marielaures Dec 13, 2017

samouss Dec 13, 2017

samouss Dec 12, 2017 •

edited

Loading

samouss Dec 12, 2017

vvo Dec 14, 2017

samouss Dec 15, 2017

vvo Dec 16, 2017

samouss Dec 17, 2017 •

edited

Loading

vvo Dec 17, 2017

vvo Dec 17, 2017

samouss left a comment •

edited

Loading

	describe('Highlighter - Highlight', () => {
	const defaultProps = {
	value: 'test',
	highlightedTagName: 'em',
	isHighlighted: false,
	nonHighlightedTagName: 'div',
	};

	it('renders a highlight', () => {
	const props = {
	...defaultProps,
	isHighlighted: true,
	};

	const wrapper = shallow(<Highlight {...props} />);

	expect(wrapper).toMatchSnapshot();
	});

	it('renders a nonhighlight', () => {
	const props = {
	...defaultProps,
	};

	const wrapper = shallow(<Highlight {...props} />);

	expect(wrapper).toMatchSnapshot();
	});
	});

feat(Highlight): support array of strings #715

feat(Highlight): support array of strings #715

Conversation

marielaures commented Dec 12, 2017 • edited by samouss Loading

algobot commented Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Haroenv Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

samouss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouss Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouss Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouss left a comment • edited Loading

Choose a reason for hiding this comment

marielaures commented Dec 12, 2017 •

edited by samouss

Loading

algobot commented Dec 12, 2017 •

edited

Loading

Haroenv Dec 12, 2017 •

edited

Loading

samouss Dec 12, 2017 •

edited

Loading

samouss Dec 17, 2017 •

edited

Loading

samouss left a comment •

edited

Loading