Skip to content

Effects: Add tests for jQuery.Tween#2326

Merged
gnarf merged 2 commits into
jquery:masterfrom
gnarf:effects-tests
Jun 27, 2015
Merged

Effects: Add tests for jQuery.Tween#2326
gnarf merged 2 commits into
jquery:masterfrom
gnarf:effects-tests

Conversation

@gnarf

@gnarf gnarf commented May 18, 2015

Copy link
Copy Markdown
Member

Covers $.Animation and $.Tween hook/extension points.

See #2340 for the compat branch version

Comment thread src/effects/Tween.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing space before )

Copy link
Copy Markdown
Member

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.

@gnarf

gnarf commented May 19, 2015

Copy link
Copy Markdown
Member Author

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.

Comment thread .jscsrc Outdated

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member

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:

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member Author

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.

@gnarf gnarf force-pushed the effects-tests branch 2 times, most recently from bb1cb30 to a0679da Compare May 21, 2015 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

9 participants