Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component presenter class #26

Closed
wants to merge 65 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
28386a4
Initial presenter development
tombeynon Sep 17, 2015
8181e18
TODO: Dummy component class
tombeynon Sep 17, 2015
b8e8cba
Require component classes in dummy
tombeynon Sep 17, 2015
2e30bc5
Define attributes in class
tombeynon Sep 18, 2015
53fb802
Tests
tombeynon Sep 18, 2015
89eed1e
Readme update
tombeynon Sep 18, 2015
9da1b65
Added defaults example
tombeynon Oct 16, 2015
2ca764e
Moved component lookup to presenter class
tombeynon Oct 16, 2015
a7dadc3
Remove rails helpers
tombeynon Oct 16, 2015
861e9d1
Added convenience method for properties
tombeynon Oct 19, 2015
4dd2cb3
Rails autoload and .descendants doesn't work
tombeynon Oct 19, 2015
1a5727a
Attribute defaults should pre-populate hash
tombeynon Oct 19, 2015
a0adcec
Deep merge attribute defaults
tombeynon Oct 19, 2015
c7021f3
Public methods will be exposed to views
tombeynon Oct 19, 2015
683f9c2
Remove broken #attributes tests (now private)
tombeynon Oct 22, 2015
00f75d6
Handle autoloading in engine initializer
tombeynon Oct 22, 2015
c64c8e5
Fix Hound issues
tombeynon Oct 22, 2015
e0eab2d
Cleaned up attributes
tombeynon Oct 22, 2015
422a279
Single to double quotes
tombeynon Oct 22, 2015
ca3bf20
Redundant self
tombeynon Oct 22, 2015
bf12ab0
Line length
tombeynon Oct 22, 2015
728363a
Trailing whitespace, align params..
tombeynon Oct 22, 2015
df31920
Use camelize instead of classify
tombeynon Oct 26, 2015
383a056
Rename Attributes to Properties
tombeynon Oct 30, 2015
cdf1845
Better `component_for` method
tombeynon Oct 30, 2015
1cf37a4
Presenters now create a view context to render
tombeynon Oct 30, 2015
692f606
Update CardComponent
tombeynon Nov 2, 2015
070e4bf
Tidy presenter and define property methods
tombeynon Apr 14, 2016
17cafd5
Use controller view context over self
tombeynon Apr 14, 2016
b4f6748
Fix load issues
tombeynon Apr 14, 2016
a15192a
Expose component public methods
tombeynon Apr 14, 2016
9aa58a0
Tidy
tombeynon Apr 14, 2016
6036a8d
Some actual presenter tests
tombeynon Apr 14, 2016
4c24333
Initial presenter development
tombeynon Sep 17, 2015
286d77b
TODO: Dummy component class
tombeynon Sep 17, 2015
9882966
Require component classes in dummy
tombeynon Sep 17, 2015
84d614e
Define attributes in class
tombeynon Sep 18, 2015
bdb2c9c
Tests
tombeynon Sep 18, 2015
3dd5821
Readme update
tombeynon Sep 18, 2015
b46365a
Added defaults example
tombeynon Oct 16, 2015
dabb6c7
Moved component lookup to presenter class
tombeynon Oct 16, 2015
1c59089
Remove rails helpers
tombeynon Oct 16, 2015
9dcaabb
Added convenience method for properties
tombeynon Oct 19, 2015
baf1c08
Rails autoload and .descendants doesn't work
tombeynon Oct 19, 2015
897a5c4
Attribute defaults should pre-populate hash
tombeynon Oct 19, 2015
8bc298c
Deep merge attribute defaults
tombeynon Oct 19, 2015
16662fe
Public methods will be exposed to views
tombeynon Oct 19, 2015
13eeeaa
Remove broken #attributes tests (now private)
tombeynon Oct 22, 2015
636b6b2
Handle autoloading in engine initializer
tombeynon Oct 22, 2015
6502d0d
Fix Hound issues
tombeynon Oct 22, 2015
1ade24d
Cleaned up attributes
tombeynon Oct 22, 2015
c80984f
Single to double quotes
tombeynon Oct 22, 2015
e255d33
Redundant self
tombeynon Oct 22, 2015
682021a
Line length
tombeynon Oct 22, 2015
678f022
Trailing whitespace, align params..
tombeynon Oct 22, 2015
2176069
Use camelize instead of classify
tombeynon Oct 26, 2015
2a458b3
Rename Attributes to Properties
tombeynon Oct 30, 2015
0698f5f
Better `component_for` method
tombeynon Oct 30, 2015
33b208b
Merge pull request #4 from tombeynon/presenter-rendering
tombeynon Apr 15, 2016
c6e6ea2
Fix hound issues
tombeynon Apr 15, 2016
2449dcc
Update README with new presenter behaviour
tombeynon Apr 15, 2016
a7625d8
Use to_partial_path now we control the view context
tombeynon Apr 17, 2016
166906e
Protect Presenter public methods more reliably
tombeynon Apr 17, 2016
c0c9ea5
Attempt to fix Rails 3 error
tombeynon Apr 18, 2016
cde8031
Rollback to_partial_path work
tombeynon Apr 18, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ app/
header.css
header.js
header.yml
header_component.rb # optional
```

Keep in mind that you can also use `scss`, `coffeescript`, `haml`, or any other
Expand All @@ -58,11 +59,33 @@ coffee-script as long as you have these preprocessors running on your app.
```erb
<!-- app/components/header/_header.html.erb -->
<div class="header">
<h1>This is a header component with the title: <%= properties[:title] %></h1>
<h3>And subtitle <%= properties[:subtitle] %></h3>
<h1>This is a header component with the title: <%= title %></h1>
<h3>And subtitle <%= subtitle %></h3>
<ul>
<% links.each do |link| %>
<li><%= link %></li>
<% end %>
</ul>
</div>
```

```ruby
# app/components/header/header_component.rb
class HeaderComponent < MountainView::Presenter
attributes :title, :subtitle
attribute :links, default: []

def title
properties[:title].titleize
end
end
```

Including a component class is optional, but it helps avoid polluting your
views and helpers with presenter logic. Any properties you pass when rendering
the component will also be exposed to the view as local variables, and you can
access them using the `properties` method in your component class and views.

### Using components on your views
You can then call your components on any view by using the following
helper:
Expand Down
3 changes: 2 additions & 1 deletion app/helpers/mountain_view/component_helper.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
module MountainView
module ComponentHelper
def render_component(slug, properties = {})
render "#{slug}/#{slug}", properties: properties
component = MountainView::Presenter.component_for(slug, properties)
render partial: component.partial, locals: component.locals
end
end
end
1 change: 1 addition & 0 deletions lib/mountain_view.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require "mountain_view/version"
require "mountain_view/configuration"
require "mountain_view/presenter"

module MountainView
def self.configuration
Expand Down
64 changes: 64 additions & 0 deletions lib/mountain_view/presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
module MountainView
class Presenter
class_attribute :_attributes, instance_accessor: false
self._attributes = {}

attr_reader :slug, :properties
alias_method :props, :properties
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this alias being used anywhere? Does it make sense to keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component classes end up with a lot of references to the properties hash and I found I wanted a shorter alias. I think it does, but maybe I'm just being lazy 😉

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see! I think I'd be fine to remove it, and people can always alias in in their inherited presenters if they want.


def initialize(slug, properties={})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surrounding space missing in default value assignment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surrounding space missing in default value assignment.

@slug = slug
@properties = properties
end

def partial
"#{slug}/#{slug}"
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of naming this method to_partial_path? Ref: https://robots.thoughtbot.com/rendering-collections-in-rails

It's a bit more Rails-y, and that way we can use render component, locals: component.locals in the render_component method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice solution, hadn't even thought of that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, unfortunately Rails does something a bit special with to_partial_path. If the controller is namespaced (such as MountainView::StyleguideController) then it'll namespace the to_partial_path with the controller namespace, resulting in mountain_view/component/component/. See the following method..

https://github.com/rails/rails/blob/20559834e4959531f02f85cac433127c2cc52b20/actionview/lib/action_view/renderer/partial_renderer.rb#L482

The only way I can see to prevent this behaviour would be to override partial_path entirely. Not awful, but maybe overkill for what we're trying to achieve.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, didn't know about that!
Yeah, overriding partial_path is definitively overkill. As is, partial, should be perfectly fine then.

Thanks for the research!


def locals
locals = build_hash
locals.merge(properties: locals)
end

def attributes
@attributes ||= properties.keys.inject({}) do |sum, name|
sum[name] = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the default a hash here?
Does it break stuff if we just don't provide a value or make it nil?

sum
end.merge(self.class._attributes)
end

private

def build_hash
attributes.inject({}) do|sum, (k, v)|
sum[k] = attribute_value(k)
sum
end
end

def attribute_value(key)
attribute = attributes[key] || return
value = respond_to?(key) ? send(key) : properties[key]
value || attribute[:default]
end

class << self
def attributes(*args)
opts = args.extract_options!
attributes = args.inject({}) do |sum, name|
sum[name] = opts
sum
end
self._attributes = self._attributes.merge(attributes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant self detected.

end

alias_method :attribute, :attributes
end

def self.component_for(slug, properties={})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surrounding space missing in default value assignment.

klass = descendants.find{|d| d.to_s.demodulize == "#{slug.to_s.classify}Component" }
klass ||= self
klass.new(slug, properties)
end
end
end
3 changes: 2 additions & 1 deletion test/dummy/app/components/card/_card.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<%- if properties[:description] %>
<p><%= properties[:description] %></p>
<%- end %>
<%= foobar %>
<div class="card__content__data">
<%- if properties[:data] && properties[:data].any? %>
<%- properties[:data].each do |data| %>
Expand All @@ -23,4 +24,4 @@
<% end %>
</div>
</div>
</div>
</div>
19 changes: 19 additions & 0 deletions test/dummy/app/components/card/card_component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class CardComponent < MountainView::Presenter
attributes :title, :description, :link, :image_url
attribute :data, default: []
attribute :foobar

def title
['Foobar', properties[:title]].join(' - ')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

end

def foobar
h.content_tag(:p, "Foobar!!")
end

private

def h
ActionController::Base.helpers
end
end
3 changes: 3 additions & 0 deletions test/dummy/config/initializers/mountain_view.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
MountainView.configure do |config|
config.included_stylesheets = ["global"]
end

# include component classes
Dir.glob('app/components/**/*.rb') { |f| require_relative "../../#{f}" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be possible to add a line to the MountainView::Engine to add the components path as a Rails autoload_path, so users don't have to add anything to their initializers.

Something in the lines of the following should work: (haven't tested it)

# in lib/mountain_view/engine.rb
initializer "mountain_view.append_component_paths" do |app|
  app.autoload_paths += [MountainView.configuration.components_path]
end

It'd probably have to include the sub-directories, so Rails doesn't fail to load the files because there's no namespace.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree, to be honest I'd put that in temporarily and totally forgotten about it. Thanks for the pointer.

48 changes: 48 additions & 0 deletions test/mountain_view/presenter_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
require "test_helper"
class InheritedPresenter < MountainView::Presenter
attributes :title, :description
attribute :data, default: []

def title
"Foo#{properties[:title].downcase}"
end
end

class MountainViewPresenterTest < ActiveSupport::TestCase
def test_partial
presenter = MountainView::Presenter.new("header")
assert_equal "header/header", presenter.partial
end

def test_locals
properties = {name: 'Foo', title: 'Bar'}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

presenter = MountainView::Presenter.new('header', properties)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

assert_equal add_properties(properties), presenter.locals
end

def test_attributes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes is a private method, which causes this test (and another one below) to raise an error, and fail. In general, private methods should not be tested.
Is there a reason this is tested? Was attributes public before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I hastily made the attributes method private in a recent commit, it interferes if you pass an attributes key in render_component. I don't think this needs to be a public method so I'll remove the broken tests

properties = {name: 'Foo', title: 'Bar'}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

presenter = MountainView::Presenter.new('header', properties)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

assert_equal({name: {}, title: {}}, presenter.attributes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.

end

def test_inherited_locals
properties = {name: 'Foo', title: 'Bar'}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

presenter = InheritedPresenter.new('header', properties)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

presenter_properties = {title: 'Foobar', description: nil, data: []}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

expected_properties = properties.merge(presenter_properties)
assert_equal add_properties(expected_properties), presenter.locals
end

def test_inherited_attributes
properties = {name: 'Foo', title: 'Bar'}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

presenter = InheritedPresenter.new('header', properties)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

presenter_attributes = {title: {}, description: {}, data: {default: []}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.

expected_attributes = {name: {}, title: {}}.merge(presenter_attributes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.

assert_equal expected_attributes, presenter.attributes
end

def add_properties(properties)
properties.merge(properties: properties)
end
end