Component presenter class #26
Conversation
Added base presenter class and integrated No external changes
Added defaults Added view helpers `h`
Ideally HTML will stay in the views. Can be include in subclasses if it's needed.
|
|
||
| def test_inherited_attributes | ||
| properties = {name: 'Foo', title: 'Bar'} | ||
| presenter = InheritedPresenter.new('header', properties) |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| attr_reader :slug, :properties | ||
| alias_method :props, :properties | ||
|
|
||
| def initialize(slug, properties={}) |
There was a problem hiding this comment.
Surrounding space missing in default value assignment.
Use safe_constantize to force autoload
Components are singularized otherwise
Confusing with two names
Presenter view context
|
@kitop @devnacho Rebased and fixed any Hound issues. I've also updated the Readme a little more. Happy to create a new PR (or squash some commits) if you want this one cleaned up a bit more. Let me know your thoughts on how this all works. I've still got a couple more suggestions (I think it would work a lot better to rename Presenter to Component, so want your opinions on that), but overall I'm pretty happy with where this is. |
|
@tombeynon thank you very much for the PR! 😀 Code looks good. @kitop something to comment or we merge it? |
|
@tombeynon this looks fantastic!! Thank you so much!! Yes, rebasing would be nice, if you don't mind. Happy to hear the rest of the suggestions. I agree that renaming |
|
@kitop I definitely think Component would be better, I find myself using it accidentally since it makes more sense semantically. Personally I think the current Component class could be in a Apart from that, I guess my only other concern is whether the README is clear enough about these changes. If you guys are happy, so am I. I need to squash some of the commits (rebased already), but am currently without a computer, so I'll sort that on Monday at the latest. |
|
@tombeynon no hurries. And thanks again for all the work. I think the README is clear, but I'm a bit biased as I've been following the PR and reading the code. I'll have a few colleagues read through it, and if any suggestions I'll let you know or we can update. |
| self.class.delegate meth, to: :_component | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected.
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Final newline missing.
First off - loving MountainView. The front-end team at the agency I work for went nuts over it, but we did find a lot of presentation logic was ending up in the erb files. We usually end up with a large presentation layer in our projects so I figured we could extend MountainView to handle a lot of this logic, and keep the f'enders happy too.
I noticed this had been raised recently (#23) so decided to see if I could come up with a solution you guys would want to merge into master. I've kept everything completely backwards compatible, and tried to keep it as simple and contained as possible. See what you think, I'm definitely open to suggestions.