-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Effects: Add tests for jQuery.Tween #2326
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
Conversation
src/effects/Tween.js
Outdated
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.
missing space before )
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.
We've settled on nodeType as the DOM node discriminator... I'd say that means replacing !tween.elem.style with tween.elem.nodeType !== 1 here, and tween.elem.style with tween.elem.nodeType === 1 in set. There's also some rearranging to make them more compressible, but that can come after the logic update.
|
Thanks @arschmitz for the spaces nits, super useful. I'm thinking about splitting this into its own test file so I can turn on the jscs. |
.jscsrc
Outdated
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.
+cc @jquery/core What is the max line length?
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.
Do not change jscsrc, it was already defined correctly - http://contribute.jquery.org/style-guide/js/#spacing
Lines should be no longer than 80 characters, and must not exceed 100 (counting tabs as 4 spaces). There are 2 exceptions, both allowing the line to exceed 100 characters:
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.
100, search the string "Lines should be" here https://contribute.jquery.org/style-guide/js/ :)
oh crap, I didn't refresh my page.
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.
I tried searching the style guide for it, but my find fu was poor I suppose...
So if it was "defined correctly" why wasn't it erroring on my long lines?
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.
Reverted this change, found that the new version of jscs works - but grunt doesn't run it...
😦 -- such a pain to follow our style guide right now when the tooling isn't up to date.
bb1cb30 to
a0679da
Compare
Gruntfile.js
Outdated
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.
This now checks actual unit tests too, right?
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.
Just the ones I added now, the others are still dirty.
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.
Well, it says "only test helpers"
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.
you suggesting I just nuke the comment?
// TODO: Enable all tests, for now, test helpers and the ones that pass are includedThere 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.
something like that seems alright.
|
Anyone want to +1 this? |
|
LGTM |
|
Looks good 👍 |
Covers
$.Animationand$.Tweenhook/extension points.See #2340 for the compat branch version