-
Notifications
You must be signed in to change notification settings - Fork 531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(snippet): add a snippet widget to be able to highlight snippet results #1797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly suggestions (nothing really blocking)
}); | ||
return <span className="ais-Highlight">{reactHighlighted}</span>; | ||
export default function Highlight(props) { | ||
return <Highlighter path="_highlightResult" {...props}/>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you notice that path
can be overriden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The path is not a prop documented in the Highlight/Snippet widget so I figured out it was ok to just forward the props. Do you think it's an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ok 👍
} | ||
|
||
Highlight.propTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we don't need to add the proptypes in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if I add them then you get two warnings if a props is missing. From the <Highlight>
, and from the <Highlighter>
. But I can definitely add them back.
attributeName, | ||
hit, | ||
}) { | ||
if (!hit) throw new Error('`hit`, the matching record, must be provided'); | ||
|
||
const highlightObject = get(hit._highlightResult, attributeName); | ||
const highlightObject = get(hit[path], attributeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly a technicality but here path
is not really a path but the property that contains the highlight structure. I suggest we change path to a name that explicitly state that the user can't go deep, idea: highlightProperty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, I'll change for highlightProperty
fe51a4d
to
2145b8e
Compare
<a name="2.2.0"></a> # [2.2.0](v2.1.0...v2.2.0) (2017-01-17) ### Bug Fixes * **clear:** clearing wasn't working with too+ same type facets selected (#1820) ([a9a2364](a9a2364)) * **connectSearchBox:** handle `defaultRefinement` (#1829) ([7a730e2](7a730e2)), closes [#1826](#1826) * **Instantsearch:** Update all props on InstantSearch (#1828) ([2ed9b49](2ed9b49)) * **InstantSearch:** add specific `react-instantsearch ${version}` agent (#1844) ([a1113bc](a1113bc)) * **SFFV:** correct propTypes and add missing default values (#1845) ([a4c1b31](a4c1b31)) * **test:** add missing Snippet and Highliter snapshot ([4accce5](4accce5)) * **widgets:** replace setImmediate use with Promise use when update is needed (#1811) ([17e2497](17e2497)) ### Features * **Menu, connectMenu:** add search for facet values (#1822) ([a6c513e](a6c513e)) * **snippet:** add a snippet widget to be able to highlight snippet results (#1797) ([2aecc40](2aecc40)) * **widgets:** add transformItems to be able to sort and filter (#1809) ([ba539f0](ba539f0))
You can now write this:
<Snippet attributeName="description" hit={hit}/>
and have highlighting for snippet attributes.#1776