Refactor the has_many form #2645

seanlinsley · 2013-11-02T22:14:31Z

First and foremost, this PR moves the JS onclick calls out into their own dedicated file. This also adds before & after hooks so you can more easily update dynamic parts of the form, for example if you're using a plugin like Chosen for interactive dropdowns:

$ ->
  $('a.button.has_many_add').on 'has_many_add:after', ->
    $(@).siblings('fieldset:last').find('select').chosen()

There's one other change here: I switched around the text used for has_many deletion because it didn't make sense to me that delete was applied to something that hadn't yet been saved, while remove was used for saved records.

Update:

After talking with @igorbernstein, I updated the jQuery hooks to be bound to the parent div, and now the after-add hook returns the just-added fieldset:

$ ->
  $('.has_many').on 'has_many_add:before', ->
    alert 'foo'
  $('.has_many').on 'has_many_add:after', (e, fieldset)->
    fieldset.find('select').chosen()
    alert 'bar'

Another update:

You can now short-circuit creation of new has_many fieldsets:

$ ->
  $(document).on 'has_many_add:before', '.has_many', (e)->
    if $(@).children('fieldset').length >= 3
      alert "you've reached the maximum number of items"
      e.preventDefault()

Finally:

#2679 updated the CSS classes so now you use has_many_container instead of has_many

seanlinsley · 2013-11-02T22:20:19Z

app/assets/javascripts/active_admin/components/has_many.js.coffee

+
+    @has_many_index ||= $(@).siblings('fieldset').length
+    regex             = new RegExp $(@).data('placeholder'), 'g'
+    $(@).before unescape_html $(@).data('html').replace regex, ++@has_many_index


This relies on a really weird (but useful) thing: you can set properties on DOM elements, and they'll store that information for you. So if you come back to the same element, that data will still be there. I'm doing that here to store the current index, because people could potentially have multiple has_many forms inside the same parent form.

I'm not sure if this is the correct, I think you will end up w. duplicate indices if a fieldset is removed. Also I don't quite understand what you mean about storing the index and how it helps with multiple has_many forms

I'm not sure if this is the correct, I think you will end up w. duplicate indices if a fieldset is removed.

If a fieldset is removed, the number will just keep counting up. So you might have some numbers not used at all, but at least they won't conflict.

Also I don't quite understand what you mean about storing the index and how it helps with multiple has_many forms

If I had a global variable that stored the current index, that wouldn't act sanely if you have mutliple has_many forms. Whereas this solution binds the index onto the add button, so it's unique to every has_many form.

I see, that makes sense

On Sun, Nov 3, 2013 at 1:57 PM, Sean Linsley notifications@github.comwrote:

In app/assets/javascripts/active_admin/components/has_many.js.coffee:

@@ -0,0 +1,17 @@
+unescape_html = (html)->

String(html).replace(/&/g, '&').replace(/</g, '<').replace(/>/g,'>').replace(/"/g, '"').replace(/'/g, "'")

+$ ->

$(document).on 'click', 'a.button.has_many_remove', (e)->

e.preventDefault()

$(@).closest('.has_many_fields').remove()

$('a.button.has_many_add').click (e)->

e.preventDefault()

$(@).trigger 'has_many_add:before'

@has_many_index ||= $(@).siblings('fieldset').length

regex = new RegExp $(@).data('placeholder'), 'g'

$(@).before unescape_html $(@).data('html').replace regex, ++@has_many_index

I'm not sure if this is the correct, I think you will end up w.
duplicate indices if a fieldset is removed.

If a fieldset is removed, the number will just keep counting up. So you
might have some numbers not used at all, but at least they won't conflict.

Also I don't quite understand what you mean about storing the index and
how it helps with multiple has_many forms

If I had a global variable that stored the current index, that wouldn't
act sanely if you have mutliple has_many forms. Whereas this solution binds
the index onto the add button, so it's unique to every has_many form.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2645/files#r7391569
.

As for the actual way I'm storing that index, it's apparently a bit controversial.

Hmm, http://stackoverflow.com/questions/7200722/jquery-expando-properties

'@' refers to a DOMNode and @hasManyIndex will be an expando on that dom object so updating @hasManyIndex will by its nature update the DOM.

jQuery data uses a single expando for all the data you store for that node, avoiding dom updates for future writes

When I say "update the DOM", I'm really saying "redraw the canvas". The current approach is just updating the DOM's JavaScript runtime object for that HTML, which as far as I can tell doesn't require a redraw.

I have a working replacement but because of the way JavaScript is built, it's unfortunately hard to read:

unless index = parent.data 'has_many_index' parent.data 'has_many_index', parent.find('fieldset').length index = parent.data 'has_many_index' regex = new RegExp elem.data('placeholder'), 'g' html = elem.data('html').replace regex, index++

Another implementation would be:

if parent.data 'has_many_index' parent.data has_many_index: parent.data('has_many_index') + 1 else parent.data has_many_index: parent.find('fieldset').length regex = new RegExp elem.data('placeholder'), 'g' html = elem.data('html').replace regex, parent.data('has_many_index')

index = parent.data('hasManyIndex') || parent.find('fieldset').length parent.data('hasManyIndex', ++index);

igorbernstein · 2013-11-03T18:24:57Z

lib/active_admin/form_builder.rb

@@ -141,13 +140,10 @@ def js_for_has_many(assoc, form_block, template, new_record)
        :class       => "inputs has_many_fields",
        :for_options => { child_index: placeholder }
      }
-      js = with_new_form_buffer{ inputs_for_nested_attributes opts, &form_block }
-      js = template.escape_javascript js
+      html = CGI.escapeHTML with_new_form_buffer{ inputs_for_nested_attributes opts, &form_block }


This will cause the html to be double escaped (which I'm guessing is reason why you added a manual unescape_html function

https://github.com/seanlinsley/active_admin/blob/991cb803d2f822be373bb2e2e0cccdc05295b4a5/app/assets/javascripts/active_admin/components/has_many.js.coffee#L1

The reason that happens is because:

with_new_form_buffer returns unescaped html thats marked as html_safe

CGI.escapeHTML returns escaped html thats not marked html_safe

link_to ... {data: html} re-escape the html because its not marked save

the most succinct way to accomplish what you want is to taint the return value from with_new_form_buffer:

'' + with_new_form_buffer {... }

but to be clearer:

html = CGI.escapeHTML(...).html_safe

Ah, yeah that was the problem. Updating the PR now.

igorbernstein · 2013-11-03T20:41:54Z

would you have any interest in including sortable functionality in your pull request? or should I open up a new PR once you merge this?

seanlinsley · 2013-11-03T20:50:50Z

Not in this pull request, no. I'm really just scratching an itch here, because I couldn't stand leaving the has_many JS embedded in the HTML like that, and figured it'd then be simple to add hooks once that was done. Feel free to create a PR for that after this. 🐶

seanlinsley · 2013-11-03T21:13:46Z

Now to actually update the tests 😅

igorbernstein · 2013-11-03T21:27:34Z

app/assets/javascripts/active_admin/components/has_many.js.coffee

+    parent = elem.closest '.has_many'
+    parent.trigger 'has_many_add:before'
+
+    index = parent.data('has_many_index') || parent.find('fieldset').length - 1


to account for @shekibobo nested use case, this should be

index = parent.data('has_many_index') || parent.children('fieldset').length - 1

I'm confused, isn't that exactly what I already have?

parent.find('fieldset') vs parent.children('fieldset')

Ah. Updated.

coveralls · 2013-11-04T02:26:44Z

Coverage increased (+0.05%) when pulling fdd7d4aaeee904541bcdef052e2a6b6d2549494e on seanlinsley:refactor/has_many-form into ef166a0 on gregbell:master.

First and foremost, this commit moves the JS `onclick` calls out into their own dedicated file. This also adds before & after hooks so you can more easily update dynamic parts of the form, for example if you're using a plugin like Chosen for interactive dropdowns: ```coffee $ -> $(document).on 'has_many_add:after', '.has_many', (e, fieldset)-> fieldset.find('select').chosen() ``` There's one other change here: I switched around the text used for has_many deletion because it didn't make sense to me that delete was applied to something that hadn't yet been saved, while remove was used for already-saved records.

coveralls · 2013-11-04T02:59:18Z

Coverage increased (+0.05%) when pulling d46b67e on seanlinsley:refactor/has_many-form into 9392ee5 on gregbell:master.

Refactor the has_many form

'doh

reorganize javascript folder structure

821a84d

seanlinsley reviewed Nov 2, 2013
View reviewed changes

igorbernstein mentioned this pull request Nov 3, 2013

Improved & sortable has_many #2614

Closed

igorbernstein reviewed Nov 3, 2013
View reviewed changes

seanlinsley added a commit that referenced this pull request Nov 4, 2013

Merge pull request #2645 from seanlinsley/refactor/has_many-form

3db28da

Refactor the has_many form

seanlinsley merged commit 3db28da into activeadmin:master Nov 4, 2013

seanlinsley deleted the refactor/has_many-form branch November 4, 2013 03:11

seanlinsley mentioned this pull request Nov 4, 2013

Allow min/max dates to be set on datepicker input #2578

Merged

seanlinsley added a commit that referenced this pull request Nov 4, 2013

bugfix for #2645

18a4b48

'doh

shekibobo mentioned this pull request Nov 4, 2013

New has_many fieldset actually affects first, already-existing fieldset since #2645. #2655

Closed

seanlinsley mentioned this pull request Nov 11, 2013

Add jquery callbacks for has_many field added and removed #1767

Closed

seanlinsley mentioned this pull request Nov 22, 2013

Nested Resources replaces existing objects #2724

Closed

Uh oh!

Refactor the has_many form #2645

Refactor the has_many form #2645

Uh oh!

Conversation

seanlinsley commented Nov 2, 2013

Update:

Another update:

Finally:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorbernstein commented Nov 3, 2013

Uh oh!

seanlinsley commented Nov 3, 2013

Uh oh!

seanlinsley commented Nov 3, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 4, 2013

Uh oh!

coveralls commented Nov 4, 2013

Uh oh!

Uh oh!