Skip to content

Conversation

@gnarf
Copy link
Member

@gnarf gnarf commented May 18, 2015

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

See #2340 for the compat branch version

Copy link
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
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
Copy link
Member Author

gnarf commented May 19, 2015

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
Copy link
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
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
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
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
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
Gruntfile.js Outdated
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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"

Copy link
Member Author

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 included

Copy link
Member

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.

@gnarf
Copy link
Member Author

gnarf commented Jun 12, 2015

Anyone want to +1 this?

@timmywil
Copy link
Member

LGTM

@arthurvr
Copy link
Member

Looks good 👍

@gnarf gnarf merged commit b3b2d6c into jquery:master Jun 27, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
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