Skip to content

Add support for multiple facets in useFacetCallback and useFacetEffect #14

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

Merged
merged 12 commits into from
Dec 8, 2021

Conversation

pirelenito
Copy link
Member

@pirelenito pirelenito commented Dec 7, 2021

There was been motivated by internal use-cases that relied on adding extra useFacetMaps to achieve the same goal. This should have a lower overhead with the added benefit that now all of our hooks share the same API (facets are arrays).

Also updates the useRemoteFacetPropSetter to accept a remoteFacet instead of a facet, making the API more consistent. I did this update as I needed to touch the implementation.

I would like special attention from reviewers on the types inside the useFacetCallback implementation. Any suggestion for improvements (specially on handling the unknowns) will be very welcome.

TODO:

  • After merging we will need to update the documentation pages

Also updates the useRemoteFacetPropSetter to accept a remoteFacet instead of a facet.
@pirelenito pirelenito requested review from xaviervia and dderg December 7, 2021 11:24
@pirelenito
Copy link
Member Author

Chatted with @xaviervia and he suggested we extend this API modification to the useFacetEffect. Will do that before we can proceed with the PR.

@pirelenito pirelenito marked this pull request as draft December 7, 2021 15:36
@pirelenito pirelenito changed the title Add support for multiple facets in useFacetCallback Add support for multiple facets in useFacetCallback and useFacetEffect Dec 8, 2021
@pirelenito
Copy link
Member Author

It is ready for review again 🎉

@pirelenito pirelenito marked this pull request as ready for review December 8, 2021 09:20
@pirelenito pirelenito merged commit 97cdedc into main Dec 8, 2021
@pirelenito pirelenito deleted the use-facet-callback-multiple-facets branch December 8, 2021 12:58
pirelenito added a commit that referenced this pull request Dec 10, 2021
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.

3 participants