-
Notifications
You must be signed in to change notification settings - Fork 516
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(Insights): Insights inside Instantsearch #3598
Conversation
ac1c448
to
765ac85
Compare
Deploy preview for instantsearchjs ready! Built with commit a7da8ed |
Deploy preview for instantsearchjs ready! Built with commit 77a7174 |
a80fd78
to
97625ca
Compare
5fa170f
to
6ac560a
Compare
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.
Nice to see this feature in InstantSearch.js!
I left a bunch of comments already. I think there's room for improvements in most of the errors and warnings.
It seems like Prettier wasn't run correctly.
54cc993
to
7b0595e
Compare
d70b780
to
b183fb0
Compare
89af5c0
to
b3e152c
Compare
b3e152c
to
6b15b1c
Compare
src/connectors/infinite-hits/__tests__/connectInfiniteHits-test.js
Outdated
Show resolved
Hide resolved
b8e063b
to
3aa8b4e
Compare
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.
I added suggestions for error messages. If you go for some of them, you might need to re-run Prettier and update the snapshots.
@algolia/instantsearch-for-websites will squash this to ease the conflict solving. It shouldn't matter because we squash before merging anyway. |
1146f41
to
b5a6850
Compare
feat(Insights): add withInsightsClient connector wrapper This PR adds a withInsightsClient which is an HOC for connectors. ```js const connectHitsWithInsightsClient = withInsightsClient(connectHits) ``` This connector will be used by default in the widget Hits and InfiniteHits to wrap connectHits and connectInfiniteHits respectively. When this PR is merged and released we can start using withInsightsClient in the flavours. feat(Insights): add template helpers feat(Insights): add withInsightsListener This commit adds withInsightsListener which - wraps Hits and InfiniteHits component, - listens to inner clicks targetting elements with data-insights attributes - calls the insights client exposed by `withInsightsClient` ```js const HitsWithInsightsListener = withInsightsListener(Hits) ``` feat(Insights): allow passing insightsClient to instantsearch instance feat(Insights): add listener to Hits feat(Insights): add listener to InfiniteHits feat(Insights): add storybook example for hits feat(Insights & typescript): type all the things extract *WithInsightsListeners components to upper scope rename withInsightsClient to withInsights feat(Insights): fix positions in infiniteScroll feat(Insights): extract addAbsolutePositions to make it connector responsibility unshorten variable name ev -> event rename isFirstRendering -> isFirstRender type mouse event avoid inline return clean unsused comment avoid inline return inline cast document.querySelector<HTMLElement> prettier things remove document maniputation in tests remove inferrable types in readDataAttributes use post modern typing type return values Apply suggestions from code review Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> add space before describe change inferPayload signature to use named params remove perf comment add space before Unmounter Apply suggestions from code review Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> Update src/lib/__tests__/insights-client-test.js Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> remove param mutation absolute imports before relative convert to typescript client and listener tests factorize Without inside 'types/utils' simplify types typo feedback: expose addAbsolutePosition in utils feedback: improved error message add type on serializedPayload Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> casting div.firstElementChild with as keyword Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> feedback: move casting on another line feedback: improve error message on incorrect data-insights-payload json string prettier all the things feedback: add type for Hits React component fix typescript file linting add query id feat(insights): add __queryID to connectHits & connectInfiniteHits simplified tests feat(insights): encode payload to base64 typo Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> typo Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> feedback: fix insightsClient defaulting to jest.fn() feedback: remove unused beforeEach feedback: group expect statements for payloads typo Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> feedback: add quotes around objectID Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> feedback: rename WidgetParams -> TWidgetParams feedback: move clickAnalytics: true to configure feedback: use toHaveBeenNthCalledWith feedback: remove extra check for insightClient callability feedback: remove proptypes fix: typo inline setup function fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: error message formatting Co-Authored-By: tkrugg <tkrugg@users.noreply.github.com> fix: update snapshots after error message reformatting
e6db95e
to
6244430
Compare
This is mimicking the way we allow usage for `highlight` and `snippet`. ```js Instantsearch.widgets.hits({ // ... templates: { item(hit) { return ` <h2> ${hit.name} </h2> <button ${ Instantsearch.insights('clickedObjectIDsAfterSearch', { eventName: "Add to favorite", objectIDs: [hit.objectID] }) }> Add to favorite </button> `; }, }, }); ```
…sWithInsights The intention is to make it easier and more straight forward to create custom InfiniteHits and Hits with the connector. If we exposed `withInsights` directly, this is what we'd need to decide on a definitive name that implies it works only on Hits (like withHitsInsights) ```js const connectHitsWithInsights = Instantsearch.connectors.withHitsInsights(Instantsearch.connectors.connectHits); const connectInfiniteHitsWithInsights = Instantsearch.connectors.withHitsInsights(Instantsearch.connectors.connectInfiniteHits); ``` If we expose `connectHitsWithInsights` and `connectInfiniteHitsWithInsights` directly, we can keep `withInsights` totally private, and all custom components example are more simple.
4f36403
to
b8d4c0c
Compare
# [3.4.0](v3.3.0...v3.4.0) (2019-04-17) ### Bug Fixes * **storybook:** fix Hierarchical menu separator in Breadcrumb story ([#3695](#3695)) ([b3bf8ac](b3bf8ac)) * **tools:** use commonjs in bump-package-version.js ([#3699](#3699)) ([6a6dbe1](6a6dbe1)) * **types:** fix wrong typing in getWidgetState ([#3693](#3693)) ([b3c2154](b3c2154)) * **types:** remove unused Without type ([#3694](#3694)) ([656d000](656d000)) ### Features * **infiniteHits:** add previous button ([#3675](#3675)) ([2e6137b](2e6137b)) * **Insights:** Insights inside Instantsearch ([#3598](#3598)) ([387f41f](387f41f))
Summary
This PR follows up on #3547 and aims to introduce 3 things
withInsightsClient
: connector wrapper that exposes insights client to connectorshelpers/insights
: which adds helpers that generatedata-insight-*
attributeswithInsightsListener
: which wraps Hits and InfiniteHits components and listens to clicks on elements containingdata-insights-*
attributes.Result
WIP