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

use-raf hook: animation hook for react using requestAnimationFrame #623

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sparkenstein
Copy link

What new hook does?

Checklist

  • Have you read contribution guideline?
  • Does the code have comments in hard-to-understand areas?
  • Is there an existing issue for this PR?
  • Have the files been linted and formatted?
  • Have the docs been updated?
  • Have the tests been added to cover new hook?
    • NOTE: need more tests
  • Have you run the tests locally to confirm they pass?

@Sparkenstein
Copy link
Author

Sparkenstein commented Jan 31, 2022

Few questions:

  • I am not really good with Jest, need to add more tests. please suggest new tests and approaches.
  • I was referring react-use test cases for useRaf, it uses raf-stub which is written in JS. so couldn't use it in tests as types are not available for it

@Sparkenstein
Copy link
Author

any plan to merge this?

@xobotyi
Copy link
Contributor

xobotyi commented Feb 2, 2022

i have several notices on implementation, but I'm ill ATM, will do the review later.

* @param delay delay in milliseconds after which to start re-rendering component
* @returns number
*/
export function useRaf(ms = 1e12, delay = 0): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

ms should have no default value, also, it is better to rename in to duration

* @param delay delay in milliseconds after which to start re-rendering component
* @returns number
*/
export function useRaf(ms = 1e12, delay = 0): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This hook should have handles to stop and start re-rendenring process, start should apply optional rerender duration.
  2. In implementation it should reuse useRafCallback hook, that is already implemented in this package.
  3. elapsed should use useSafeState, since it is updated asynchronously - there is a chance, that component will be unmounted before state update.

@xobotyi
Copy link
Contributor

xobotyi commented Feb 10, 2022

I was referring react-use test cases for useRaf, it uses raf-stub which is written in JS. so couldn't use it in tests as types are not available for it

Tests don't require raf-stub since Jest provides pretty much all needed functionality, see https://github.com/react-hookz/web/blob/master/src/useRafCallback/__tests__/dom.ts

@xobotyi xobotyi added 🎂 new hook New hook added 👎 invalid This doesn't seem right labels Mar 8, 2022
@SCG82
Copy link

SCG82 commented Sep 8, 2022

Please consider streamich/react-use#2215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👎 invalid This doesn't seem right 🎂 new hook New hook added
Development

Successfully merging this pull request may close these issues.

3 participants