sortable has_many #2656

igorbernstein · 2013-11-04T15:40:36Z

This is a revival of #2614, #881, #2117, #880
Its a still a work in progress, but I wanted to post this early on to make sure I'm on the right path and this doesn't slip through the cracks.

TODO:

~~force initial ordering~~
~~add an indicator icon to show that the fieldsets can be dragged~~
~~add css to change the cursor to indicate that the fieldset is draggable~~
add tests
- http://www.dixis.com/?p=626
- http://forum.jquery.com/topic/fire-events-to-simulate-drag-n-drop
~~add docs~~
~~deal with nested sortable has_many fieldsets~~

igorbernstein · 2013-11-04T15:50:17Z

@seanlinsley, are the jasmine tests used? or should I implement cucumber/capybara tests?

seanlinsley · 2013-11-04T16:37:19Z

We have Jasmine, but have never actually used it. I'd very much like to have JS tests, but for some reason Jasmine is throwing an error when I try running it.

igorbernstein · 2013-11-12T06:39:51Z

This only thing thats missing here are tests. But the jasmine tests are broken & there doesn't appear to be any js tests implemented via cucumber/capybara. Unfortunately I don't have time to open up that can of worms (figuring out why jasmine is broken and then fixing all the existing tests because they haven't been run in awhile).

@seanlinsley Would you merge this w/o js tests? or should I spin this off into a separate gem?

seanlinsley · 2013-11-12T16:30:45Z

Would you merge this w/o js tests?

I'll answer your question with a question: is there anything I can do to get you to write tests? Any way I can accommodate your skills and schedule?

or should I spin this off into a separate gem?

I'd hope not! IMO the whole create "spin-off gems extending AA" thing was just a way for the previous maintainers to be lazy. We don't need more fragmentation.

igorbernstein · 2013-11-12T22:00:21Z

I have a couple of competing goals:

I want to make some meaningful contributions to open source software (i.e. View layer overhaul #2689)
I need to implement a couple of admin interfaces for a couple of clients

One of the admin interfaces that a client signed off on was a sortable has_many UI, which was implemented in my fork, but was antiquated by the changes in #2679. Hence this PR, which rebases that functionality on current FormBuilder structure.

My client's next priority is to get an autocomplete widget for selecting editions for their homepage. Since they will need both functionalities, I will need to have one of these features merged in master or available in a gem or living in my own fork of active admin.

I don't have a problem with writing tests for this functionality, but I'm concerned about the amount of work necessary to get my tests running along side the existing js tests & passing again, so I can't commit to working on that until december. The path of least resistance for me would be: to get this merged into master, open up a PR for the autocomplete feature, base my client's admin off that PR, then deal with jasmine tests as a whole in december.

shekibobo · 2013-11-12T22:08:19Z

@igorbernstein Just FYI, Select2 is actually pretty simple to integrate in ActiveAdmin on its own. In my apps, I simply add select2 to all select fields. Then for other types of fields, like tags and tokenizable fields, I just write a new Formtastic input and JS to get the behavior I need. While I think it would be great to have these features integrated fully into ActiveAdmin, it is by no means a requirement to have these features integrated within an application instance.

seanlinsley · 2013-11-12T22:48:07Z

@igorbernstein if you're going to be around for the long haul then I think we can forgo the tests for now. Later this month I'll try tackling the JS testing issue so we have a framework that is actually operational.

Is this PR fully functional? I'll try it out locally and we can hopefully get it merged tonight.

seanlinsley · 2013-11-12T22:54:15Z

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

+  parent.children('fieldset').each ->
+    fieldset = $(@)
+    destroy = fieldset.find ':input[name$="[_destroy]"]'
+    sortable_input = fieldset.find ":input[name$='[#{input_name}]']:eq(0)"


Is the :eq(0) necessary? Are there going to be multiple inputs with the same name?

I'm kinda hair-splitting here but line 7 uses " '' " while line 8 uses ' "" '. One way or another, I think they should be consistent

the eq(0) is necessary in the nested has_many case
sorry about the inconsistency, i'll switch it to " ' ' " because I need to interpolate the input_name

In that case wouldn't it make more sense to have a direct descendent CSS rule?

Its a little ugly:

sortable_input = fieldset.find "> ol > li > :input[name$='[#{input_name}]']"

also the _destroy finder has the same issues

I added the correct but ugly selectors for the time being

ok, so the simplest ideas I have are:

the closestDescendant plugin

or just to localize the crazy in a helper:

recompute_positions = (parent)-> .... destroy = find_fieldset_input fieldset, "_destroy" sortable_input = find_fieldset_input fieldset, input_name find_fieldset_input = (parent, name)-> parent.find "> ol > .input > :input[name$='[#{name}]']"

edit: i prefer no. 2

Personally I prefer it like it is, with long but obvious CSS selectors. As the behavior grows it might be worth it to abstract things some more, but for now I think what you already have in the PR is appropriate.

I'm cool with that, I added a comment to avoid future head scratching

coveralls · 2013-11-13T02:34:49Z

Coverage decreased (-0.18%) when pulling 3e6d51e on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

seanlinsley · 2013-11-13T17:32:03Z

docs/5-forms.md

+    f.inputs :taggings, sortable: :position do |t|
+      t.input :tag
+      t.input :position
+    end


This would be has_many, not inputs

sorry that was sloppy. fixed now

seanlinsley · 2013-11-13T17:42:53Z

One problem I noticed is that if there's another f.inputs block on the page, the arrows on the right side don't scope to just the f.has_many area

igorbernstein · 2013-11-13T23:57:59Z

Not sure what you mean about arrows not scoping to just the .has_many area...but maybe I accidentally fixed it in the other changes?

I moved the arrows to the top right corner instead of right center, to avoid losing them on longer forms and mixing them up with other arrows from nested has_many fieldsets
I'm trying out @shekibobo original approach of just making the arrows draggable
I fixed the invalid mark up ol > span.handle to ol > li.handle

coveralls · 2013-11-14T00:02:37Z

Coverage decreased (-0.21%) when pulling 77ceec9 on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

seanlinsley · 2013-11-14T00:33:56Z

This is a screenshot I took earlier today illustrating the problem. Haven't tried your updated code yet.

igorbernstein · 2013-11-14T00:35:49Z

Can you post the form definition code for that screenshot?

On Wed, Nov 13, 2013 at 7:33 PM, Sean Linsley notifications@github.comwrote:

This is a screenshot I took earlier today illustrating the problem.
Haven't tried your updated code yet.

[image: screen shot 2013-11-13 at 11 41 55 am]https://f.cloud.github.com/assets/688886/1537367/56fa0bb2-4cc4-11e3-95b6-ba3024977824.png

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2656#issuecomment-28449254
.

seanlinsley · 2013-11-14T00:39:31Z

It was a f.inputs block, followed by a f.has_many block. Let's see if I still have the code around...

seanlinsley · 2013-11-14T00:41:14Z

Here it is:

  form do |f|
    f.inputs :body
    f.has_many :taggings, sortable: :position do |t|
      t.input :tag
      t.input :position
    end
    f.actions
  end

igorbernstein · 2013-11-14T00:48:33Z

thanks! just pushed a fix

On Wed, Nov 13, 2013 at 7:41 PM, Sean Linsley notifications@github.comwrote:

Here it is:

form do |f|
f.inputs :body
f.has_many :taggings, sortable: :position do |t|
t.input :tag
t.input :position
end
f.actions
end

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2656#issuecomment-28449785
.

coveralls · 2013-11-14T00:58:32Z

Coverage decreased (-0.18%) when pulling 5ec157c on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

seanlinsley · 2013-11-14T02:03:51Z

Cool.

What about centering the handle vertically to the fieldset? With this CSS:

.has_many_container .handle {
  top: calc(50% - 3em / 2);
}

You get a nicely centered handle:

Also, it'd be great if the developer didn't have to manually build the input for the sortable attribute, and it was built automatically.

igorbernstein · 2013-11-14T02:59:03Z

never ran across calc before...its pretty cool
the original css was vertically centering the arrows with:

    margin-top: -1.5em;
    top: 50%;

But I changed it because the arrows gets lost on longer fieldsets
Also, with nested sortable is complete confusion:

igorbernstein · 2013-11-14T02:59:27Z

👍 👍 on auto generating sortable input

seanlinsley · 2013-11-14T03:03:01Z

the problem with the image you attached is that the nested has_many fieldsets don't have a big enough margin on the right to give room for the parent's handle

igorbernstein · 2013-11-14T03:16:23Z

i moved the arrows back to the vertical center, but could use some help making the margins look ok...ui & css aren't not a strong suit of mine

coveralls · 2013-11-14T03:23:47Z

Coverage decreased (-0.21%) when pulling aa24c69 on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

seanlinsley · 2013-11-14T03:26:02Z

Could you use the calc setting I gave earlier? That will get it exactly in the middle, accounting for the height of the icons.

coveralls · 2013-11-14T03:31:25Z

Coverage decreased (-0.21%) when pulling bd5279b on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

igorbernstein · 2013-11-14T03:34:14Z

pushed, though for personal curiosity, I don't understand why the negative margin gives different results from the calc

ie. why is calculation of:

calc(50% - HEIGHT / 2);

different from

top: 50%;
margin-top:- HEIGHT/2;

seanlinsley · 2013-11-14T03:41:32Z

In my browser these both have the same effect:

top: calc(50% - 3em / 2);

/* and */

top: 50%;
margin-top: -1.5em;

coveralls · 2013-11-14T03:45:13Z

Coverage decreased (-0.21%) when pulling bd5279b on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

igorbernstein · 2013-11-14T03:56:09Z

I guess we had a miscommunication somewhere around commit aa24c69 & your 'Could you use the calc setting I gave earlier? That will get it exactly in the middle, accounting for the height of the icons.' comment.

But I don't think it really matters.

When you have a free moment, would you mind suggesting how I can improve the right margins to avoid arrow soup when nesting sortables?

seanlinsley · 2013-11-17T04:26:50Z

docs/5-forms.md

@@ -64,6 +64,12 @@ ActiveAdmin.register Post do
      end
    end
    f.inputs do
+      f.has_many :taggings, sortable: :position do |t|
+        t.input :tag
+        t.input :position


Can you update this since the t.input :position part is no longer needed?

This still needs to be updated

seanlinsley · 2013-11-17T04:33:17Z

I just noticed an interesting bug. Given this form:

  form do |f|
    f.semantic_errors *f.object.errors.keys
    f.inputs
    f.has_many :posts, allow_destroy: true do |o|
      o.input :title
      o.has_many :taggings, sortable: :position do |t|
        t.input :tag
      end
    end
    f.actions
  end

A just-added post's taggings aren't sortable.

coveralls · 2013-11-17T18:18:56Z

Coverage decreased (-0.21%) when pulling a44c440 on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

igorbernstein · 2013-11-17T18:48:39Z

I cherry picked the css changes from your PR.

Good catch on b8bba4b , but I think we should generalize your solution a bit.
How would you feel about generalized initialize_content, teardown_content events? The idea being that dynamic overlays & has_many widgets can generate the same events & all widgets will just work™.

# application
$ ->
  $(document).trigger('initialize_content')

# datepicker
$(document).on('initialize_content', (e) ->
  $(@).find('.datepicker').datepicker()

$(document).on('teardown_content', (e) ->
  $(@).find('.datepicker').datepicker('destroy')

# has_many (in addition to the existing  has_many_*:* events)
$(document).on 'click', 'a.button.has_many_add', (e)->
  # ..
  fieldset.trigger('initialize_content')

# sortable
$(document).on 'initialize_content' (e)->
  $('.has_many_container[data-sortable]').sortable()

seanlinsley · 2013-11-17T18:59:08Z

How about we get this PR merged first, in a state where it's fully functional? In general I'm all for standardizing the way our JavaScript works.

igorbernstein · 2013-11-17T19:28:06Z

I think you can merge this, I'll open up a separate PR to deal with the generalized widget initialization I mentioned earlier.

coveralls · 2013-11-17T19:33:15Z

Coverage decreased (-0.33%) when pulling b59e82c8dfdb2dd3888419a7a7aed8876a3ddd1a on igorbernstein:has_many-sortable into 12b19a7 on gregbell:master.

seanlinsley · 2013-11-17T19:40:56Z

Hmm, 21 commits for 100 changes... Would you mind squashing this either into one commit or a handful of commits?

coveralls · 2013-11-17T19:52:39Z

Coverage decreased (-0.21%) when pulling 7cbcf32 on igorbernstein:has_many-sortable into 131c3a4 on gregbell:master.

seanlinsley · 2013-11-17T19:53:02Z

Awesome. Thanks for working on this! 💙

sortable has_many

igorbernstein · 2013-11-17T19:54:54Z

Thanks for helping :)

coveralls · 2013-11-17T20:02:22Z

Coverage remained the same when pulling 7cbcf32 on igorbernstein:has_many-sortable into 131c3a4 on gregbell:master.

seanlinsley · 2013-11-17T20:19:00Z

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

+      parent.trigger 'has_many_add:after', [ fieldset ]
+
+  $(document).on 'change','.has_many_container[data-sortable] :input[name$="[_destroy]"]', ->
+    recompute_positions $(@).closest '.has_many'


Wait, what exactly is the purpose of this? Why would you need to recompute positions when the user clicks a checkbox?

because we don't want to include the ones marked for destruction to be included in the count...

item0
position: 0

item1
position: ''
_destroy: true

item2
position: 1

Oh I see. Ignore me :)

Babbz75 · 2016-11-22T19:22:52Z

@kirel Thank you!

For acts_as_list people wondering why the list is rotating you need to start the sort at 1. i.e. sortable_start: 1 otherwise it bumps the zero to one and then with two positions at 1 it will bump the first one to the bottom. At least I believe that's what was happening to me

This was referenced Nov 9, 2013

has_many breaks when embedded in an inputs block #2659

Closed

Handle huge associations #2692

Closed

shekibobo mentioned this pull request Nov 12, 2013

Sortable has-many forms: Revive PR #881 #2117

Closed

seanlinsley reviewed Nov 12, 2013
View reviewed changes

seanlinsley reviewed Nov 13, 2013
View reviewed changes

seanlinsley reviewed Nov 17, 2013
View reviewed changes

sortable has-many

7cbcf32

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

Merge pull request #2656 from igorbernstein/has_many-sortable

00f97ac

sortable has_many

seanlinsley merged commit 00f97ac into activeadmin:master Nov 17, 2013

seanlinsley reviewed Nov 17, 2013
View reviewed changes

igorbernstein deleted the has_many-sortable branch November 17, 2013 20:23

seanlinsley mentioned this pull request Nov 22, 2013

Nested Resources replaces existing objects #2724

Closed

Uh oh!

sortable has_many #2656

sortable has_many #2656

Uh oh!

Conversation

igorbernstein commented Nov 4, 2013

Uh oh!

igorbernstein commented Nov 4, 2013

Uh oh!

seanlinsley commented Nov 4, 2013

Uh oh!

igorbernstein commented Nov 12, 2013

Uh oh!

seanlinsley commented Nov 12, 2013

Uh oh!

igorbernstein commented Nov 12, 2013

Uh oh!

shekibobo commented Nov 12, 2013

Uh oh!

seanlinsley commented Nov 12, 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!

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!

coveralls commented Nov 13, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanlinsley commented Nov 13, 2013

Uh oh!

igorbernstein commented Nov 13, 2013

Uh oh!

coveralls commented Nov 14, 2013

Uh oh!

seanlinsley commented Nov 14, 2013

Uh oh!

igorbernstein commented Nov 14, 2013

Uh oh!

seanlinsley commented Nov 14, 2013

Uh oh!

seanlinsley commented Nov 14, 2013

Uh oh!

igorbernstein commented Nov 14, 2013

Uh oh!

coveralls commented Nov 14, 2013

Uh oh!

seanlinsley commented Nov 14, 2013

Uh oh!

igorbernstein commented Nov 14, 2013

Uh oh!

igorbernstein commented Nov 14, 2013

Uh oh!

seanlinsley commented Nov 14, 2013

Uh oh!

igorbernstein commented Nov 14, 2013

Uh oh!

coveralls commented Nov 14, 2013

Uh oh!

seanlinsley commented Nov 14, 2013

Uh oh!

coveralls commented Nov 14, 2013