search

jferris / effigy

Ruby views without a templating language
http://rdoc.info/projects/jferris/effigy
MIT License
206 stars 6 forks source link

Remove the selection proxy object #18

Open sgrif opened 11 years ago

sgrif commented 11 years ago

I've replaced the selection proxy object, and replaced it with a simple clone. The proxy object wasn't really pulling its weight, and it was locking us into having a selector at the beginning of every method signature, which made things like #first impossible

This PR also removes the optional selector (e.g. .text('h1', 'foo')) as an argument to all of the various methods, in favor of .find('h1').text('foo')

jferris commented 11 years ago

Does it make sense to move a bunch of the manipulation stuff into Selection instead of removing it? It seems like this is moving in the direction of having a mono-class library. Now that you can't call text with a selector, it doesn't really make sense to call text before find, and moving them out of View would prevent that.

sgrif commented 11 years ago

Yeah, I think everything related to current_context wants to live in another class. It'd be cool to do it in a way that exposes the enumeration more, as well. Do you think it would make sense to do that as a separate PR? Since it probably won't end up being a proxy object, so the existing code from selection will still go away regardless.

jferris commented 11 years ago

I think I'd want to see that the approach works before merging. I agree that the proxy approach isn't great, and that most of the internals of Selection should go away, but I worry that merging this as-is doesn't improve the API; it just replaces the old unintuitive API with a new unintuitive API. Does that make sense?

sgrif commented 11 years ago

Yeah, I agree with you. I'll take a swing at it tomorrow.

On Wed, May 8, 2013 at 3:30 PM, Joe Ferris notifications@github.com wrote:

I think I'd want to see that the approach works before merging. I agree that the proxy approach isn't great, and that most of the internals of Selection should go away, but I worry that merging this as-is doesn't improve the API; it just replaces the old unintuitive API with a new unintuitive API. Does that make sense?

— Reply to this email directly or view it on GitHubhttps://github.com/jferris/effigy/pull/18#issuecomment-17635212 .

Thanks, Sean Griffin

sgrif commented 11 years ago

@jferris Do you think that the block API with find still makes sense given these changes?

jferris commented 11 years ago

No, not really, especially if you can perform multiple manipulations on a selector chain (ie find('h1).add_class('title').text(page.title)).