-
Notifications
You must be signed in to change notification settings - Fork 18
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
Rename useFacetEffect to useFacetLayoutEffect and add a new useFacetEffect #30
Conversation
…f useFacetLayoutEffect
There is now a handful of code that is duplicated between We ended up deciding not to do so for now, however. We decided to prioritized keeping the code easily readable and to keep the logic between different functions completely separated, even if it is similar. |
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.
Looks great, thanks!
@@ -6,7 +6,7 @@ module.exports = { | |||
'react-hooks/exhaustive-deps': [ | |||
'error', | |||
{ | |||
additionalHooks: '(useFacetEffect|useFacetMap|useFacetMemo|useFacetCallback)', | |||
additionalHooks: '(useFacetLayoutEffect|useFacetEffect|useFacetMap|useFacetMemo|useFacetCallback)', |
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!
The name of the
useFacetEffect
function is slightly misleading in the current implementation, as it is actually using React'suseLayoutEffect
as an underlying function. Thus, we have renamed the function touseFacetLayoutEffect
and added a new function calleduseFacetEffect
in its place that actually uses React'suseEffect
.We have updated the documentation to reflect this change in a separate pull request (#31).