jonathangeiger / kohana-jelly

See the link below for the most up-to-date code
https://github.com/creatoro/jelly
MIT License
146 stars 34 forks source link

Extending Fields requires duplicate views #1

Closed banks closed 14 years ago

banks commented 14 years ago

Currently Field::input() only looks for a view with the same name as the field class.

That means, for example, Field_Email has to have an email.php view which is actually identical to the string.php view.

I think it would be better if Field objects had an $_input_view property that is set manually so that fields can inherit parent's views and only need specify one extra bit of meta data (in the field class) if they need an alternate view.

Would also save on the string manipulation and fetching of class names each time an input was fetched although this isn't really a big deal either way.

In this scenario, Jelly_Field_String would set $_input_view to be 'fields/string' and then Jelly_Field_Email wouldn't have to do anything to use the same input view. If the user wanted different HTML for an email input, they could override Field_Email and set $_input_view to be something else.

banks commented 14 years ago

I'm posting this so we have a sensible way to keep track of and discuss issues like this by the way - not because I expect anyone else to fix it!

jonathangeiger commented 14 years ago

I actually built Jelly for a scaffolding system, so this is a change I wouldn't really like to see. Even though alot of these are the same in their default state, I can see where having that separation (without having to specify it) is useful.

banks commented 14 years ago

Perhaps it would work to check parent classes until a template is found. Not sure how this would impact performance but it seems wrong to me to end up with duplicate views in the module just so the user can override the views rather than override the class and change the view property.

What do you think?

banks commented 14 years ago

I'll make these changes.

jonathangeiger commented 14 years ago

Sounds good.

banks commented 14 years ago

Fixed by 0f648cbff31e4792e6fe9afe4cace97bbc592ab7. Jon, can you merge this to master when you are happy.

Note that I also fixed a bug in this commit which makes input() method in master currently unusable.

jonathangeiger commented 14 years ago

Looks great.