yuche / vue-strap

Bootstrap components built with Vue.js
http://yuche.github.io/vue-strap/
MIT License
4.71k stars 932 forks source link

Is your nodelist.js an extension of my library? #437

Open eorroe opened 7 years ago

eorroe commented 7 years ago

Hey, I just found this file and majority is the same as my library, I'm wondering if this code was an extension to it. Which is awesome just wondering, it looks almost exactly the same. I no longer develop for the web so it's interesting finding this

eorroe commented 7 years ago

Yea I just looked at more of the code, the Error messages are the same, methods are pretty much doing the same, and variable names are the same. I'm not completely sure what your version does, but I see a lot of the same thing.

This is pretty awesome, glad to know my code is being used out there because I have no idea :(

fabd commented 7 years ago

Hi eorroe =)

Since you're here some comments that come to mind:

Is that safe? I thought I read somewhere it's not guaranteed that indexes are returned in numerical order when using for .. in.. or did that change with ES2015 ? And I assume in that one spot, you may rely on getting the nodes in exactly the order you'd expect them to come from the selector (ie. divs in a container coming in the same order they are in the DOM).

wffranco commented 7 years ago

@eorroe this nodelist was based on your, but I made a lot of changes and added event handling... I take it because we try that this implementation work without jquery and your nodelist is lighter. But in v2 maybe will disappear :v

fabd commented 7 years ago

Interesting what do you plan to use in v2 for general utility?

I like your additions, jquery style event handling.

Could you explain what the blur ones are for?

wffranco commented 7 years ago

@fabd about the for...in, the NodeList object can't be called directly, we handle NodeListJS, you can check this in the last lines. If the values don't fulfill any of the above conditions, node will receive a list of arguments, which will always have numerical indexes.

About the each method, I try to match the standard forEach method, but looks like another part of the code overwrite the method, so the fast solution was that :see_no_evil:

About the blur event, it work only on focusable elements (input, textarea, button), so for example if we click outside a select component, the blur event never trigger. The onblur event handle that kind of things, is not a real blur, is a click outside event handler... in vue2 version I convert this in a directive.

eorroe commented 7 years ago

Thanks @wffranco I see some interesting things you did in the code. If I ever get to refactoring this code and doing anything with it. I'll take some of what you've done. I don't develop for the web anymore but I still develop in JS and have improved my JS way more so if I ever go back to that code I'll do some interesting refactoring and a small breaking change. Not a fan of the asArray idk why I did that, should have just made it a toArray method.

fabd commented 7 years ago

Btw, I was wondering what part of the language is that from where you use get asArray () { ... } ? I can't seem to Google it. I've never seen it before.

eorroe commented 7 years ago

Their called getters, and setters that example is a getter. When I used Object.defineProperty as well I'm defining the getters/setters with a method call

fabd commented 7 years ago

@eorroe Hi again sorry to bother but would you have a link to the asArray syntax? I just can't find anything on it.

I understand very well the basics of getters / setters and how they work in NodeList.js, but after all kind of Google searches I can't find an article that explicitly address the "asArray" keyword as used here:

 get asArray () {
    return ArrayProto.slice.call(this)
  }

Thanks

eorroe commented 7 years ago

asArray was just me making that up what I prefer is a method, I don't remember why I chose to do that but a toArray method is better:

toArray() {
  return ArrayProto.slice.call(this)
}
fabd commented 7 years ago

Hehe :) I'm still confused >_> The NodeList.js actually compiled.

wffranco commented 7 years ago

@fabd NodeList is an object but not an array, so if you need to handle it as an array you use that function.

But usually you won't need to use this.

fabd commented 7 years ago

@wffranco Alright I got it. I misread the code. It's a getter. Gotcha. >_> I'm beginning to think I need to quit my career, lol.