Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improved performance, browser support, bugfixes #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improved performance, browser support, bugfixes #3

wants to merge 3 commits into from

Conversation

bcherny
Copy link

@bcherny bcherny commented Jun 23, 2012

Changelog:

  • Minimized repaints by eliminating style queries on every step of the
    drag
  • Generally improved performance
  • Improved browser support
  • Standardized event names (start -> dragstart, stop -> dragend)
  • Lots of bugfixes (eliminated outline dragging, fixed
    border/margin/zindex querying)

@bcherny
Improved performance, browser support, bugfixes
9e52e7d 
Changelog:

* Minimized repaints by eliminating style queries on every step of the
drag
* Generally improved performance
* Improved browser support
* Standardized event names (start -> dragstart, stop -> dragend)
* Lots of bugfixes (eliminated outline dragging, fixed
border/margin/zindex querying)
@gtramontina
Copy link
Owner

Hey man. First of all, thanks for the contribution.

Before I merge your pull request, I have a few considerations about it...

  • I ran the tests here and a great part of them are failing, could you please either fix the failures or rewrite the tests so they reflect the current behavior? And also add some tests around your improvements. To run the tests, simply open the file test/test.html on your browser, it'll run all existing tests in draggable.test.js.
  • I have an app using the current draggable version working, so I simply replaced my version with yours and it didn't work... It blows up with the following error right when I click the element: "Uncaught ReferenceError: element is not defined - draggable.js:148"

Thanks a lot!

@bcherny
Copy link
Author

bcherny commented Jun 28, 2012

Hi Guilherme,

I've actually fixed the script up a bit more, and turned it into a class so you can have multiple instances of draggables on the same page. I'll try to add tests along with the new changes sometime this weekend if that works.

Cheers,
-Boris

On Jun 26, 2012, at 10:00 PM, "Guilherme J. Tramontina"[email protected] wrote:

Hey man. First of all, thanks for the contribution.

Before I merge your pull request, I have a few considerations about it...

  • I ran the tests here and a great part of them are failing, could you please either fix the failures or rewrite the tests so they reflect the current behavior? And also add some tests around your improvements. To run the tests, simply open the file test/test.html on your browser, it'll run all existing tests in draggable.test.js.
  • I have an app using the current draggable version working, so I simply replaced my version with yours and it didn't work... It blows up with the following error right when I click the element: "Uncaught ReferenceError: element is not defined - draggable.js:148"

Thanks a lot!

Reply to this email directly or view it on GitHub:
#3 (comment)

@gtramontina
Copy link
Owner

We can already have multiple draggables on the same page, it is just a
matter of calling "draggable()" with the different elements.
Anyway, I'll wait for your next pull request. :-)

Thanks.

On Thu, Jun 28, 2012 at 6:26 PM, eighttrackmind <
[email protected]

wrote:

Hi Guilherme,

I've actually fixed the script up a bit more, and turned it into a class
so you can have multiple instances of draggables on the same page. I'll try
to add tests along with the new changes sometime this weekend if that works.

Cheers,
-Boris

On Jun 26, 2012, at 10:00 PM, "Guilherme J. Tramontina"<
[email protected]> wrote:

Hey man. First of all, thanks for the contribution.

Before I merge your pull request, I have a few considerations about it...

  • I ran the tests here and a great part of them are failing, could you
    please either fix the failures or rewrite the tests so they reflect the
    current behavior? And also add some tests around your improvements. To run
    the tests, simply open the file test/test.html on your browser, it'll run
    all existing tests in draggable.test.js.
  • I have an app using the current draggable version working, so I simply
    replaced my version with yours and it didn't work... It blows up with the
    following error right when I click the element: "Uncaught ReferenceError:
    element is not defined - draggable.js:148"

Thanks a lot!

Reply to this email directly or view it on GitHub:
#3 (comment)

Reply to this email directly or view it on GitHub:
#3 (comment)

bcherny added 2 commits June 29, 2012 12:59
@bcherny
Bugfixes, more features, improved performance
da61d9a 
This is a bit more robust than previous versions, but is also a bit
larger (in terms of file size). I think the tradeoff is worth it, as the
new features are necessary when using the draggable in any but the
simplest applications.

Note that the js files should be included at the *end of the body* in
order to avoid onDomReady sniffing. I've also removed the AMD
integration because I'm not familiar with the syntax, could you add it
back? I'm also not familiar with the unit testing framework you're
using, but I have tested the script manually in all supported browsers.
@bcherny
Update draggable.js
e7768c8 
Touch support
@dumblob
Copy link

dumblob commented Jan 20, 2022

Any news here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bcherny @gtramontina @dumblob