-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
test: update animation click test #1053
Conversation
What is this change doing? The test is still disabled? |
Yep. The test was flaky by itself, so I updated it in the hopes we can now pass it, but that revealed two issues (see the comment). |
Whats the plan to pass this test? |
const animateLeft = () => { | ||
const button = document.querySelector('button'); | ||
button.style.left = window.atLeft ? '${containerWidth - buttonSize}px' : '0px'; | ||
console.log('set ' + button.style.left); |
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.
nit: unneeded console log
expect(await page.evaluate('window.clicked')).toBe(2); | ||
expect(await page.evaluate('document.querySelector("#button").style.left')).toBe('0px'); | ||
await page.evaluate(transition => { | ||
window.setInterval(animateLeft, transition); |
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.
If we care specifically about the rafs, then this should probably be animated on raf right?
I'm not sure what we are testing, but if I was reviewing this code for a website I would say this is broken. You should always animate on raf, not setInterval.
Co-Authored-By: Joel Einbinder <joel.einbinder@gmail.com>
Co-Authored-By: Joel Einbinder <joel.einbinder@gmail.com>
No description provided.