Skip to content
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

refactor(bindEvent): move listener inside components #5528

Merged

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Mar 6, 2023

Summary

This was needlessly abstracted with a HOC, not allowing us to use this same function inside onClick of an individual hit, which could help with deduplicating sendEvent + bindEvent

I didn't remove the withInsightsListener HOC, as it could (in theory) be used externally.

Result

  • no empty div around hits/infiniteHits
  • Hits/InfiniteHits components own their event life cycle completely
  • better segmentation of v1 "insights helper" API

This was needlessly abstracted with a HOC, not allowing us to use this same function inside onClick of an individual hit, which could help with deduplicating sendEvent + bindEvent

I didn't remove the `withInsightsListener` HOC, as it could (in theory) be used externally.
@Haroenv Haroenv requested review from a team, FabienMotte and aymeric-giraudet and removed request for a team March 6, 2023 14:18
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 6, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4c1b558:

Sandbox Source
InstantSearch.js Configuration
react-instantsearch-app Configuration
example-react-instantsearch-hooks-default-theme Configuration
example-vue-instantsearch-default-theme Configuration

@Haroenv Haroenv requested a review from FabienMotte March 8, 2023 14:21
Copy link
Contributor

@FabienMotte FabienMotte left a comment

Choose a reason for hiding this comment

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

Nice! 👏

@Haroenv Haroenv merged commit b4e9504 into feat/automatic-insights Mar 8, 2023
@Haroenv Haroenv deleted the refactor/automatic-insights-bindEvent branch March 8, 2023 14:54
Haroenv added a commit that referenced this pull request Mar 8, 2023
* refactor(bindEvent): move listener inside components

This was needlessly abstracted with a HOC, not allowing us to use this same function inside onClick of an individual hit, which could help with deduplicating sendEvent + bindEvent

I didn't remove the `withInsightsListener` HOC, as it could (in theory) be used externally.

* remove unused data
Haroenv added a commit that referenced this pull request Apr 18, 2023
* refactor(bindEvent): move listener inside components

This was needlessly abstracted with a HOC, not allowing us to use this same function inside onClick of an individual hit, which could help with deduplicating sendEvent + bindEvent

I didn't remove the `withInsightsListener` HOC, as it could (in theory) be used externally.

* remove unused data
Haroenv added a commit that referenced this pull request Apr 24, 2023
* refactor(bindEvent): move listener inside components

This was needlessly abstracted with a HOC, not allowing us to use this same function inside onClick of an individual hit, which could help with deduplicating sendEvent + bindEvent

I didn't remove the `withInsightsListener` HOC, as it could (in theory) be used externally.

* remove unused data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants