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.

Sign up for GitHub

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

Jump to bottom

Issue 96 #195

Merged
merged 8 commits into from Sep 21, 2016
Merged

Issue 96 #195

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
049c4e2
add view methods for creating/updating forms
jphager2 Sep 8, 2016
6855ed6
add previously removed comment
jphager2 Sep 8, 2016
2e143ff
revert previous view changes
jphager2 Sep 11, 2016
769487a
create Pakyow::Presenter::Form for manipulating form views
jphager2 Sep 11, 2016
a3b8192
refactor Pakyow::Presenter::Form
jphager2 Sep 12, 2016
bd2a720
remove method_missing from Presenter::Form
jphager2 Sep 14, 2016
48d5c57
pass view to block for Form actions
jphager2 Sep 14, 2016
6c48c57
calling form action block in consistant way
jphager2 Sep 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pakyow-presenter/lib/pakyow/presenter/base.rb
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

require 'pakyow/presenter/view_store'
require 'pakyow/presenter/view'
require 'pakyow/presenter/form'
require 'pakyow/presenter/template'
require 'pakyow/presenter/page'
require 'pakyow/presenter/container'
Expand Down
65 changes: 65 additions & 0 deletions pakyow-presenter/lib/pakyow/presenter/form.rb
View file
Open in desktop
@@ -0,0 +1,65 @@
module Pakyow
module Presenter
class Form

attr_reader :form_view, :context
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename form_view to view. It's already scoped within the form, so it seems redundant.


def initialize(view, context)
@form_view = view
@context = context
end

def create(*binding_args, &block)
bind_data(binding_args, block) do |routes, view|
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't obvious from the name of bind_data that it yields routes and view. I can't come up with a better name, but thought I'd throw it out and see if you had any ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, tough. Yielding routes here doesn't give us much, so maybe it can be moved.

Off the top of my head, what about bind_form_action instead?

view.attrs.action = routes.path(:create, request_params) if routes
end
end

def update(*binding_args, &block)
bind_data(binding_args, block) do |routes, view|
if routes
route_params = request_params.merge(
:"#{scoped_as}_id" => view.attrs.__send__('data-id').value
)

view.attrs.action = routes.path(:update, route_params)
end

view.prepend(
View.new('<input type="hidden" name="_method" value="patch">')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the HTML string to a constant.

)
end
end

private

def bind_data(binding_args, block)
bind(*binding_args) do |view, data|
routes = Router.instance.group(scoped_as)

yield(routes, view)

return if block.nil?

if block.arity == 1
view.instance_exec(&block)
else
block.call(view, data)
end
end
end

def request_params
context.request.params
end

def method_missing(method, *args, &block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some thoughts on why we need this? I had assumed that any changes to the form would occur inside the block for create / update but perhaps there's a reason we should allow more access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is just to allow users to make further changes on the order of normal view manipulation. It may not be strictly necessary, but it seemed consistent with View#bind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Thought about it for a bit, but what about nested forms?

<form data-scope="album">
  <input data-prop="name" type="text" />
  <div data-scope="image">
    <input data-prop="name" type="text" />
  </div>
</form>
data = { id: 1, name: 'a', images: [{ id: 1, name: 'b' }, { id: 2, name: 'c' }] }
view.form(:album).update(data) do |view, data|
  view.scope(:image).apply(data[:images]) do |fields, image|
    fields.prop(:name).attrs.name = "album[images][#{image[:id]}[name]"
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or do you think that this would be better placed in a binding?

Copy link
Contributor

Choose a reason for hiding this comment

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

You wouldn't need method_missing here, right? Just change bind_form_action to call view.bind and the form's view will be yielded to the block instead of the form object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. My original idea was for form to act like a view, but with extra form actions, but since the user can access the view maybe that can be the solution to the problem above as well, so that the example above would simply be:

data = { id: 1, name: 'a', images: [{ id: 1, name: 'b' }, { id: 2, name: 'c' }] }
form = view.form(:album)
form.update(data)
form.view.scope(:image).apply(data[:images]) do |fields, image|
  fields.prop(:name).attrs.name = "album[images][#{image[:id]}[name]"
end

if form_view.respond_to?(method)
form_view.__send__(method, *args, &block)
else
super
end
end
end
end
end
5 changes: 5 additions & 0 deletions pakyow-presenter/lib/pakyow/presenter/view_context.rb
View file
Open in desktop
Expand Up @@ -59,6 +59,11 @@ def subject
end
end

def form(*scope_args)
view = scope(*scope_args).subject
Form.new(view, context)
end

# Pass these through, handling the return value.
#
def method_missing(method, *args, &block)
Expand Down
30 changes: 30 additions & 0 deletions pakyow-presenter/spec/integration/support/mixins/form_binding_specs.rb
View file
Open in desktop
Expand Up @@ -14,6 +14,36 @@
D
}

let(:router) { Pakyow::Router.instance }
let(:group) { Pakyow::Router.instance.group(:foo) }
let(:request) { mock_request }
let(:app_context) { Pakyow::AppContext.new(request, nil) }
let(:view_context) { Pakyow::Presenter::ViewContext.new(view, app_context) }

context 'when creating a form' do
it 'sets the action' do
expect(request).to receive(:params) { {} }
expect(group).to receive(:path) { '/foo' }.with(:create, {})
expect(router).to receive(:group) { group }.with(:foo)

view_context.form(:foo).create({})
expect(view.scope(:foo).first.instance_variable_get(:@doc).to_s).to include %(action="/foo")
end
end

context 'when updating a form' do
it 'sets the _method and action' do
expect(request).to receive(:params) { { other: 'param' } }
expect(group).to receive(:path) { '/foo/1' }.with(:update, foo_id: 1, other: 'param')
expect(router).to receive(:group) { group }.with(:foo)

view_context.form(:foo).update(id: 1)
html = view.scope(:foo).first.instance_variable_get(:@doc).to_s
expect(html).to include %(name="_method" value="patch")
expect(html).to include %(action="/foo/1")
end
end

context 'when binding to unnamed field' do
it 'sets name attr' do
view.scope(:foo).bind(unnamed: 'test')
Expand Down