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

Added 2 new options: direction constrains and sorting by distance #29

Merged
merged 2 commits into from Nov 9, 2015

Conversation

Projects
None yet
2 participants
@dogoku

dogoku commented Nov 6, 2015

This PR solves #20 and overlaps with PR #22 (but offers more functionality)

Added a new option called directionConstraints that takes an array of directions and filters the results to those that only appear in all the directions specified. This option will only work for Single Element Operations and gets ignored for anything else. Available directions: left,right,top,bottom

//Find the nearest .basic elements that are in the top left of $elem
$elem.nearest('.basic', { directionConstraints: ['left', 'top'] });

Added another new option called sort that takes a sorting order, nearest or furthest, and sorts the results based on their distance instead of the order they appear on the DOM.

//Get the furthest elements with 100px tolerance and sort by nearest (cause why not)
$elem.furthest('.basic', { sort: 'nearest', tolerance: 100 });

P.s: Nice testing setup. Was easy to hook into and add more tests

Show outdated Hide outdated index.html
@gilmoreorless

This comment has been minimized.

Show comment
Hide comment
@gilmoreorless

gilmoreorless Nov 9, 2015

Owner

Documentation and tests included, nice work! I'm loving the simplicity of the sort option. 😀

With the directionConstraints option, I like the API you've created, but does it need to be restricted to single element operations? I could see this easily working with x/y co-ordinates as well. In fact, the x/y point data are already pre-calculated for you in the point1x, point1y, point2x and point2y variables in the main body of the nearest internal function. Each compared element also has x, y, x2 and y2 variables calculated in the distance-checking loop. Re-using those would also minimise the amount of DOM lookups needed.

Owner

gilmoreorless commented Nov 9, 2015

Documentation and tests included, nice work! I'm loving the simplicity of the sort option. 😀

With the directionConstraints option, I like the API you've created, but does it need to be restricted to single element operations? I could see this easily working with x/y co-ordinates as well. In fact, the x/y point data are already pre-calculated for you in the point1x, point1y, point2x and point2y variables in the main body of the nearest internal function. Each compared element also has x, y, x2 and y2 variables calculated in the distance-checking loop. Re-using those would also minimise the amount of DOM lookups needed.

@dogoku

This comment has been minimized.

Show comment
Hide comment
@dogoku

dogoku Nov 9, 2015

It doesn't have to be, i just didn't have a lot of time to play with it and make it work with the multiple elements functionality. Unfortunately I can't promise I can get more time to spend on this, as I'm on a very tight schedule on a client project. Your plugin was the fastest way to get the functionality I was looking for, it just missed a couple of features :)

The checkDirectionConstraints function was basically a copy-paste from a proof of concept I wrote before finding your plugin, so I kept it as is, using the getBoundingClientRect, even though I noticed that x,y,x2,y2 provide the same functionality. Plus I felt it looked cleaner as it has no dependencies.

If I could provide one feedback is that your code is too "imperative". It's essentially one big function, where it could be split into smaller parts that are easier to reason with. For example, you have a lot of comments in your code where you are specifying what you are doing below. You could have instead extracted that code into it's own function, that can be tested separately and it's clear to see what its dependencies are through the arguments it gets passed.

dogoku commented Nov 9, 2015

It doesn't have to be, i just didn't have a lot of time to play with it and make it work with the multiple elements functionality. Unfortunately I can't promise I can get more time to spend on this, as I'm on a very tight schedule on a client project. Your plugin was the fastest way to get the functionality I was looking for, it just missed a couple of features :)

The checkDirectionConstraints function was basically a copy-paste from a proof of concept I wrote before finding your plugin, so I kept it as is, using the getBoundingClientRect, even though I noticed that x,y,x2,y2 provide the same functionality. Plus I felt it looked cleaner as it has no dependencies.

If I could provide one feedback is that your code is too "imperative". It's essentially one big function, where it could be split into smaller parts that are easier to reason with. For example, you have a lot of comments in your code where you are specifying what you are doing below. You could have instead extracted that code into it's own function, that can be tested separately and it's clear to see what its dependencies are through the arguments it gets passed.

@gilmoreorless

This comment has been minimized.

Show comment
Hide comment
@gilmoreorless

gilmoreorless Nov 9, 2015

Owner

No worries. I'm happy to merge this one in as-is and tweak it slightly, using your tests as a reference.

I completely agree about the code. I wrote this plugin 5 years ago and its contents have not greatly changed since then, but my coding knowledge/style has. At some point I'll get around to a full rewrite to make the code easier to understand, and also to make it a standalone library that doesn't need jQuery but can still work with it. I'm tracking that in issue #27 if you have any other suggestions.

Owner

gilmoreorless commented Nov 9, 2015

No worries. I'm happy to merge this one in as-is and tweak it slightly, using your tests as a reference.

I completely agree about the code. I wrote this plugin 5 years ago and its contents have not greatly changed since then, but my coding knowledge/style has. At some point I'll get around to a full rewrite to make the code easier to understand, and also to make it a standalone library that doesn't need jQuery but can still work with it. I'm tracking that in issue #27 if you have any other suggestions.

gilmoreorless added a commit that referenced this pull request Nov 9, 2015

Merge pull request #29 from dogoku/feature-directions-and-sorting
Added 2 new options: direction constrains and sorting by distance

@gilmoreorless gilmoreorless merged commit d31a57f into gilmoreorless:master Nov 9, 2015

@dogoku dogoku deleted the dogoku:feature-directions-and-sorting branch Jan 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment