Skip to content

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

Merged
gilmoreorless merged 2 commits intogilmoreorless:masterfrom
dogoku:feature-directions-and-sorting
Nov 9, 2015
Merged

Added 2 new options: direction constrains and sorting by distance#29
gilmoreorless merged 2 commits intogilmoreorless:masterfrom
dogoku:feature-directions-and-sorting

Conversation

@dogoku
Copy link

@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

index.html Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Missing a </code> here, which causes wonky styling for the rest of the page.

Copy link
Author

Choose a reason for hiding this comment

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

sorry bout that, updated now

@gilmoreorless
Copy link
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.

@dogoku
Copy link
Author

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
Copy link
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.

gilmoreorless added a commit that referenced this pull request Nov 9, 2015
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 feature-directions-and-sorting branch January 4, 2016 14:12
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.

2 participants