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

Implement animationFrame() yieldable #365

Merged
merged 2 commits into from
Sep 8, 2020
Merged

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Jul 26, 2020

This PR implements a animationFrame() utility that wraps the native requestAnimationFrame() API and can be used via yield animationFrame() (similar to yield timeout(1000 / 60)).

Related: #169

Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

two tiny docs things I noticed

addon/index.d.ts Outdated Show resolved Hide resolved
addon/utils.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

Cool, looks great to me. Could you add a quick test?

@Turbo87
Copy link
Contributor Author

Turbo87 commented Jul 28, 2020

Could you add a quick test?

any suggestions on how to test this? 😅

@maxfierke
Copy link
Collaborator

Could add a unit test that stubs out rAF (sinon should be available in the test suite) and asserts that the correct things are called and that the task runs to completion without error (might not even need to stub out rAF)

Adding to these existing tests too would be good:

@Turbo87
Copy link
Contributor Author

Turbo87 commented Aug 3, 2020

@maxfierke I've added some tests but I fail to understand why the TS typing tests are failing. They seem to work alright locally.

@chancancode any clues?

@Turbo87
Copy link
Contributor Author

Turbo87 commented Aug 24, 2020

@maxfierke @chancancode any ideas how to proceed? the failing tests seem unrelated to the code that I introduced here 🤔

@maxfierke
Copy link
Collaborator

Looks like those typescript tests are also failing in #368. I'll take a look at it this week and try and figure it out

@maxfierke
Copy link
Collaborator

@Turbo87 master's passing now, if you want to give this a rebase?

@Turbo87
Copy link
Contributor Author

Turbo87 commented Sep 8, 2020

hmm, seems there is still an unrelated test failing. or maybe it's just flaky. can you restart the failed test to check?

@maxfierke
Copy link
Collaborator

yeah, that's just a flaky one. I queued up a restart on that failed job but doesn't seem to be taking. I'll merge as-is

@maxfierke maxfierke merged commit 62c0890 into machty:master Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants