Skip to content

Dragging object #12

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

Closed
wants to merge 3 commits into from
Closed

Dragging object #12

wants to merge 3 commits into from

Conversation

robkinyon
Copy link

I needed a way of doing part of a dragging simulation in order to test the in-drag activities of a proprietary jQuery plugin. So, simulateDrag doesn't change its behavior, but its behavior is now addressable in parts.

@mikesherov
Copy link
Member

Thanks for contributing! In order for me to land this, you'll need to run grunt to make sure everything passes, and add some test cases to cover the new code that you wrote. Can you kindly do that? Once you do, I can give you feedback on the rest of the PR. Thanks again!

@robkinyon
Copy link
Author

Mike -

I am unable to run grunt because the grunt dependencies require grunt

0.4, but the Gruntfile forces the installation of 0.3.1. This is a new
installation running on Ubuntu 12.04. I don't know enough about Grunt to
make it work. I'm really sorry about that, but I'm not sure what the next
step would be.

Rob

On Mon, Mar 11, 2013 at 8:47 PM, Mike Sherov notifications@github.comwrote:

Thanks for contributing! In order for me to land this, you'll need to run
grunt to make sure everything passes, and add some test cases to cover
the new code that you wrote. Can you kindly do that? Once you do, I can
give you feedback on the rest of the PR. Thanks again!


Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-14752479
.

Thanks,
Rob Kinyon

@mikesherov
Copy link
Member

Thanks for the fast response, @robkinyon. Can you run the test suite locally? All you have to do is open the index file in the test directory to run the QUnit tests. In the meantime, I'll figure out the grunt issues... thanks for reporting.

@robkinyon
Copy link
Author

I opened test/index.html from my checkout in Chrome, and 23 of 23 tests
passed.

Note - test/qunit is empty. I had to copy qunit.js and qunit.css into
test/qunit/qunit. I suspect grunt would have populated those files.

On Mon, Mar 11, 2013 at 9:11 PM, Mike Sherov notifications@github.comwrote:

Thanks for the fast response, @robkinyon https://github.com/robkinyon.
Can you run the test suite locally? All you have to do is open the index
file in the test directory to run the QUnit tests. In the meantime, I'll
figure out the grunt issues... thanks for reporting.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-14753195
.

Thanks,
Rob Kinyon

@scottgonzalez
Copy link
Member

@robkinyon Grunt should work now, you'll need to pull master and run npm install again.

@robkinyon
Copy link
Author

I've updated the pull request per grunt running jslint and finding some extra commas and the like. Tests ran cleanly.

@mikesherov
Copy link
Member

Hi @robkinyon, in order for me to land this request, I'll also need some new tests showing that the functionality you added works properly. Do you think you can do that? Please let me know when you get a chance.

@robkinyon
Copy link
Author

Sorry - I just switched jobs, so I haven't had time to even think about
this. I apologize for the delay. I'll try and get to it soon.

On Tue, Apr 2, 2013 at 12:15 PM, Mike Sherov notifications@github.comwrote:

Hi @robkinyon https://github.com/robkinyon, in order for me to land
this request, I'll also need some new tests showing that the functionality
you added works properly. Do you think you can do that? Please let me know
when you get a chance.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-15796288
.

Thanks,
Rob Kinyon

@mikesherov
Copy link
Member

No apologies needed. Congrats on the new job! I'll just leave the PR open until you get around to it.

@mikesherov
Copy link
Member

Actually, now that I think about it. I'm probably not going to land this unless there's a strong need. I would love this help on some of the open issues if you'd like to tackle those, but I'm going to close this.

@mikesherov mikesherov closed this Apr 3, 2013
@robkinyon
Copy link
Author

I do have need for this on a project I'm working on, which is why I
provided the pull request. The ability to test in-drag changes is important
for my deliverables.

On Tue, Apr 2, 2013 at 5:31 PM, Mike Sherov notifications@github.comwrote:

Actually, now that I think about it. I'm probably not going to land this
unless there's a strong need. I would love this help on some of the open
issues if you'd like to tackle those, but I'm going to close this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-15811280
.

Thanks,
Rob Kinyon

@mikesherov
Copy link
Member

@robkinyon, what I meant was that there doesn't seem to be a strong community need for this, and this add a lot of complexity and indirection to the code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants