search

trailblazer / cells

View components for Ruby and Rails.
https://trailblazer.to/2.1/docs/cells.html
3.07k stars 236 forks source link

Allow class in `concept` helper. #336

Closed smathy closed 8 years ago

smathy commented 9 years ago

= concept "foo/bar" can be: = concept Foo::Bar and also = cell "foo/bar" can be: = cell Foo::BarCell

timoschilling commented 9 years ago

The same should work with the cell helper. Otherwise we have a inconsistent API.

smathy commented 9 years ago

Happy to add it, but let me explain my thoughts first and see if they change your thinking.

First that there's already a difference in the APIs between cell and concept. Also concept already has the newer, less magical API so making it even less magical by allowing the actual class constant seemed much more fitting for concept than cell. Further to that cell's magical API of adding the "_cell" suffix would mean that cell "foo/bar"would have to become cell Foo::BarCell which is internally inconsistent for cell's own API.

timoschilling commented 9 years ago

I understand what you mean, the reflection from the string argument to the class is different. But if we change only concept we have a different API between both. Until now they have both the same args. Sure there is a difference in the name notation, but I think thats the best way.

cell("foo/bar") == cell(Foo::BarCell)
concept("foo/bar") == concept(Foo::Bar)
smathy commented 9 years ago

Ok, so I refactored slightly to take out the duplicate code in both classes, so only the full_cell_name method is implemented in both classes, the camelize.constantize is only in the base class. I also renamed the name parameter to name_or_class where appropriate.

timoschilling commented 9 years ago

it look's like your are loving the close / open button :wink:

timoschilling commented 9 years ago

good work :+1:

smathy commented 9 years ago

Yeah, close/open is the only way I can trigger a new Travis run for those occasional failures. Using that @response.body.must_equal is a bad idea without a describe context. I'll send a PR to fix that too :)

smathy commented 9 years ago

Ok, so fixed that test failure in #338, could merge that to master and rebase this to get past those annoying travis false negatives.

smathy commented 9 years ago

:shipit: baby! Do you merge stuff @timoschilling - or is that only @apotonick ?

timoschilling commented 9 years ago

No thats only @apotonick, I'm only a normal user in this repo

smathy commented 9 years ago

:+1:

smathy commented 8 years ago

Bump, how about this @apotonick ?

apotonick commented 8 years ago

Why so complicated?

screenshot_2016-04-18_21-51-03

Thanks for that, @smathy ! :heart:

smathy commented 8 years ago

Did I miss some simpler way to do that?