Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Some features added #1

Open
wants to merge 10 commits into
from

Conversation

Projects
None yet
2 participants

mcginty commented May 6, 2011

  • Used some newer CSS to beautify the selection process
  • Added the hitting of the control key to select parent nodes to get hard-to-reach elements.
  • Probably a couple other things I forgot about.

Anyway, feel free to take a look and keep what you like for anyone else interested :). I renamed the namespace nodeselector just to keep it more generic; it wasn't an attempt to take credit away from you or anything like that. Feel free to change it back in your version!

~Jake

Owner

ptarjan commented May 16, 2011

Thanks for the pull request. Going through and commenting.

ptarjan commented on ns.js in b5d6667 May 16, 2011

I agree, thank you.

I think I'd prefer to test the page without jquery loaded to make sure it still works.

ptarjan commented on ns.js in d08fc7c May 16, 2011

I find this syntax a bit to "c-ish" to be in javascript as it makes it hard to read. Can you do

while (elt && elt.nodeType == 1) {

elt = elt.parentnode;
}

Also, please keep the indenting and spacing styles I have in the file, or change the whole file, don't mix and match. (space around = and +, preferring if() to ternary operators)

ptarjan commented on ns.js in d08fc7c May 16, 2011

I'm worried you're missing some functionality from the old code by shortening it here. A refactor without test cases is scary. I don't see any mention of "//" or "/html".

ptarjan commented on ns.js in d9a9f58 May 16, 2011

This was so that you can press escape and unload this library. Maybe it isn't needed... if so, remove it, don't comment it out please.

ptarjan commented on ns.js in d9a9f58 May 16, 2011

you put the old code back?

ptarjan commented on ns.js in 2903204 May 16, 2011

does this guarantee it is overtop of all elements on the page? I'm worried you might click on a background element and everything else obscures it

ptarjan commented on ns.js in f7f3f55 May 16, 2011

Hmm. I thought I tested this method and found some times it didn't load. Maybe it is working now. What browsers did you test in?

ptarjan commented on ns.js in 1db96fd May 16, 2011

Interesting. I might have gone with the up key. I don't really like pressing the ctrl key without anything else and havnig it do something.

Owner

ptarjan commented May 16, 2011

Thanks for the request. If you do one big cleanup commit and post a good test plan, then I'll merge them in.

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