Mojang / ore-ui

💎 Building blocks to construct game UIs using web tech.
https://react-facet.mojang.com/
MIT License
409 stars 18 forks source link

Rename useFacetEffect to useFacetLayoutEffect and add a new useFacetEffect #30

Closed jacobbergdahl closed 2 years ago

jacobbergdahl commented 2 years ago

The name of the useFacetEffect function is slightly misleading in the current implementation, as it is actually using React's useLayoutEffect as an underlying function. Thus, we have renamed the function to useFacetLayoutEffect and added a new function called useFacetEffect in its place that actually uses React's useEffect.

We have updated the documentation to reflect this change in a separate pull request (#31).

jacobbergdahl commented 2 years ago

There is now a handful of code that is duplicated between useFacetEffect and useFacetLayoutEffect. @MartinMoe and I had a discussion about whether we should extract parts of the code into shared functions. We could even combine certain tests or make parts of the implementation of the tests reusable between useFacetEffect and useFacetLayoutEffect.

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.