AmpersandJS / ampersand-view

A smart base view for Backbone apps, to make it easy to bind collections and properties to the DOM.
http://ampersandjs.com
MIT License
92 stars 39 forks source link

`.get` should possibly be renamed #18

Closed HenrikJoreteg closed 10 years ago

HenrikJoreteg commented 10 years ago

The method was named get from before when view wasn't based on ampersand state.

State already has a get() :cry:

But... i'm not sure it really matters since, basically no one, except those transitioning a backbone app, uses it. But even in that case, backboneview didn't have a get so, i'm not sure it really matters.

Just figured i'd raise it.

jakemhiller commented 10 years ago

Yeah, I think this should be changed to something that doesn't conflict with ampersand-state.

Maybe change it to find? Sort-of makes it seem like jQuery, but it describes the functionality well.

jrmyio commented 10 years ago

+1 on find() It feels like a better name than .get().

wraithgar commented 10 years ago

We already have precedent for a thing backbone used to have that is easily mixed back in if you are transitioning, or still want it for other reasons https://github.com/whobubble/ampersand-view-jquery-mixin I say rename it and accept mixins w/ open arms.

If it's a querySelector, I'd push for naming the method select

HenrikJoreteg commented 10 years ago

Here's what I propose:

query() - takes a regular CSS selector, returns first match (even if first match is root element) queryAll() - takes regular CSS selector, returns array of all matches (even if that includes root) queryByHook("hook-name") - builds a CSS selector for you like this: [data-hook="hook-name"] then calls query with that (returns one element). queryAllByHook("hook-name") - builds CSS selector in the same way as queryByHook then calls queryAll.

For those familiar with querySelector and querySelectorAll it's a clear mental mapping. Select has other meanings in my mind, and is a character longer. /me shrugs. I feel like query is quite understandable.

A possible, shorter alternative would be to simply use q. Then it would look like this:

q() qAll() qByHook() qAllByHook()

But, i'm leaning toward the longer version for readability and clarity.

HenrikJoreteg commented 10 years ago

Maybe the *AllByHook isn't really necessary. query() queryAll() queryHook() - note that there's no "By"

^ that looks really clean/simple to me.

legastero commented 10 years ago

Looks clean & simple to me too. Don't forget to use the ~= operator in queryHook() :)

HenrikJoreteg commented 10 years ago

yeah, good point, should have carried that over from the other discussion.

garrettn commented 10 years ago

Why no queryAllHook()? Seems like it would still come in handy.

HenrikJoreteg commented 10 years ago

@garrettn, yeah i suppose. But it's simply not something i've missed thus far building apps using the current getByRole.

latentflip commented 10 years ago

I personally feel that queryHook is a little to abstract

this.query, this.queryAll read pretty well to me. this.queryHook feels odd, it's like I'm adding a hook to a query or something. this.queryByHook seems more clear to me personally.

In context:

this.renderCollection(ItemView, this.collection, this.queryHook('item-list'))

vs

this.renderCollection(ItemView, this.collection, this.queryByHook('item-list'))

HenrikJoreteg commented 10 years ago

+1 agreed

latentflip commented 10 years ago

I'm going to merge this and publish as a release candidate so that I can begin to update other modules. If anyone has any pushback on query, queryAll, queryByHook now is the time to make noise.

HenrikJoreteg commented 10 years ago

closed in v7.0.0 published now