Make has_many nested records' Remove button uniform #2827

Open
wants to merge 17 commits into
from

6 participants

@davidstosik

Hello,
At the moment, has_many nested record form offer two ways to remove a record, depending on wether it's new (Remove button that also removes the record form from the page) or existing (simple destroy checkbox).
I propose in this pull request to render this remove feature uniform, by replacing the destroy checkbox by a Remove button similar to the one used on new records.
The destroy checkbox becomes a hidden input, which is set to true when the user clicks on Remove. The record's form is also hidden in the process.
Sortable still works.
I also updated rspec tests.

Thanks in advance for your review,

David

@davidstosik davidstosik Make has_many nested records' Remove button uniform.
The Remove button is now the same, wether it's a new or an existing record.
7e5c254
@shekibobo

Can we add an undo UI for fields that are marked for removal?

@davidstosik

Hello,

To undo removing a persisted record, you can already use the form's "Cancel" action (while there's no button for new records).
I would consider this "undo" feature not a part of the feature I'm requesting here, and think that it should go in its own pull request.

I think, if "undo" button there is, that it should work as well for new records as for existing ones. Undoing removing new records will require some changes on the way they are removed (their HTML is simply removed from the page, but it should be kept somehow, if we want to be able to restore it).

@seanlinsley seanlinsley commented on an outdated diff Dec 28, 2013
...ssets/javascripts/active_admin/lib/has_many.js.coffee
parent.trigger 'has_many_remove:before', [ to_remove ]
- to_remove.remove()
+
+ destroy_input = to_remove.find '> ol > .input > :input[name$="[_destroy]"]'
+ if destroy_input.length > 0
+ destroy_input.attr 'value', true
@seanlinsley
Active Admin member
seanlinsley added a line comment Dec 28, 2013

In JavaScript, zero isn't considered truthy. So the more idiomatic way to write this is:

if destroy_input.length
  # yes
else
  # no
@seanlinsley
Active Admin member
seanlinsley added a line comment Dec 28, 2013

Or at least, I like that style :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanlinsley seanlinsley commented on an outdated diff Dec 28, 2013
lib/active_admin/form_builder.rb
@@ -60,13 +60,12 @@ def has_many(assoc, options = {}, &block)
index = parent_child_index options[:parent] if options[:parent]
contents = block.call has_many_form, index
- if has_many_form.object.new_record?
- contents << template.content_tag(:li) do
- template.link_to I18n.t('active_admin.has_many_remove'), "#", class: 'button has_many_remove'
- end
- elsif builder_options[:allow_destroy]
- has_many_form.input :_destroy, as: :boolean, wrapper_html: {class: 'has_many_delete'},
- label: I18n.t('active_admin.has_many_delete')
+ contents << template.content_tag(:li) do
+ template.link_to I18n.t('active_admin.has_many_remove'), "#", class: 'button has_many_remove'
+ end
+
+ if !has_many_form.object.new_record? && builder_options[:allow_destroy]
+ has_many_form.input :_destroy, as: :hidden
@seanlinsley
Active Admin member
seanlinsley added a line comment Dec 28, 2013

The opposite of new_record? is persisted?, by the way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidstosik

@seanlinsley Thanks for the comments, I updated my pull request. :)

@graysonwright

👍 I was just hoping for this change -- check boxes are a very unintuitive way to remove fields. If we merge this, I can start working on an "Undo" pull request.

@seanlinsley
Active Admin member

@igorbernstein what do you think about this?

I'm hesitant to accept this because it makes the form's behavior much less obvious. Currently it's clear that a persisted record isn't actually deleted until the form is submitted, but this pull request removes that clarity by using the same UI mechanism for persisted and just-added records.

But assuming that we end up agreeing and that this gets merged, we now have an unused set of has_many translations that should be removed. @dstosik would you mind removing the active_admin.has_many_delete translation from all the locales?

@shekibobo

@seanlinsley I agree with you, which is why I would push for an undo UI as part of accepting this PR.

@davidstosik

Happy New Year to all of you, may your lives prosper, and Active Admin get better and better! 🎉

@seanlinsley Done. I removed all the translations for has_many_delete.

Now I want to talk about what you were both saying, with @shekibobo.
You're gonna need a bit more than just an undo button to explain to an ordinary user, who was already not understanding the difference between the Remove button and the destroy checkbox (may I add that many translations were using the same word for both?).

Also, to go with that "standardization" between new and existing records, I would suggest to provide an Undo button for both removals : I just added a new nested record, filled it with a lot of data (didn't save the form yet), but removed it by inadvertence, why wouldn't the form allow me to undo my mistake?

My suggestions are as follows:

  • To make the distinction clear between new (unsaved) and existing record, I would say this needs some theming. For example, a new record could have a different background color, and a sort of watermark in big prints, saying "NEW".
  • Concerning the Undo button, I suggest a single button for each "has_many" nested form in the page, that undoes last remove, and can be clicked as many times as records (new or persisted) were removed.
@davidstosik

Just so that nobody starts working on an Undo button, I'm letting you know that I'm actually working on it, and may submit a first draft soon.

@davidstosik

Any comment will be appreciated. ;)

@davidstosik

Note: I forgot to handle form submission, I'll post an update later today.

@davidstosik

Still needs tests, but this is the complete feature, with "Remove" button for all nested records, "Undo Remove", to unremove any removed record (in reverse order), and background coloration (color can be discussed ;) ) for new records.
Again, I'm open to discussion.

@seanlinsley
Active Admin member

Thanks for putting so much work into this @dstosik!

This seems to be working:

f.inputs do
  f.has_many :foos do |bar|
    bar.input :baz
  end
end

But not for this:

f.has_many :foos do |bar|
  bar.input :baz
end

Also note that the sortable form's input field is currently visible, when it shouldn't be. Like this:

screen shot 2014-01-02 at 9 23 02 am

@shekibobo

Is there any chance that we can make the Undo action/button/whatever replace the contents of the removed fieldset instead of being just one button at the bottom? Basically, instead of hiding the container itself, the button appears in place of the inputs. This way, we can undo each individually, rather than historically.

@davidstosik

@seanlinsley Thanks, I'll try to see why your second example doesn't work.
Edit: that was just a CSS problem, fixed in commit below.

Regarding the sortable form's input field, it is not visible on the pages I'm working on, and I did nothing to hide it.
From what I can see, if I pass :sortable => :position (:position being the sortable field's name), then ActiveAdmin generates a hidden <input> for it. Can you please check your code and show me how you define the sortable?

@shekibobo I don't really like the idea of replacing each removed item by an "Undo" button, as this may clutter the interface, and may not play well with sortable. Most undo interfaces that I know work historically, so I don't see that as a problem.

@davidstosik davidstosik Fix has_many_removed and has_many_new CSS.
So that it works even if f.has_many is not inside f.inputs.
89453f4
@shekibobo

My thought was that they would behave more like the checkboxes that we are intending to replace. I don't feel like it's as clear what is happening when you undo deletes. The page jumps around. And what if I want to undo a delete in a different order than I made them? I have to do it all again. And reordering should just count fieldset that aren't marked for removal. I can look into a mockup or some actual code for this in the morning.

@davidstosik

And reordering should just count fieldset that aren't marked for removal.

I know that. But what happens to those "Undo Remove" buttons when you move things around? Can you drag and drop an "Undo Remove" button if your relation is sortable? In which position will the nested entity be when you unremove it?

@shekibobo

All good questions. I'll meditate on those.

@shekibobo

In the same vain, what happens if you undo a removal after the others have been rearranged?

@davidstosik

I didn't think about it, and let the drag and drop solution handle this itself. As the removed item is completely hidden, this didn't look like a problem to me.

Now if you show an "Undo remove" button for each removed item, you'll have to handle ordering of these buttons when other items are dragged around, and also to ensure you restore the item where the Undo button was.

@shekibobo

So, a further realization is that we aren't doing anything to deal with normalized sortable indices. Right now, the assumption we make is that the sortable interface does just that - sorts. When you submit an item for deletion, everything gets submitted, and that index is just skipped, but everything is still sorted. I personally think that this will be fine in most situations. But that's my opinion.

There is a flaw in that opinion, though. Say we want to sort on page_number for some reason. What we would need to do is have some sort of normalize_sortable_indices method that gets triggered/run just before submit. Or it's on the user to write the normalization code before saving.

Either way, I think that doing the singular undo would still work just fine because of the fact that either we do or do not normalize sort orders anyway.

In my mind, the removed fields would still technically be sortable. The fields would be hidden and replaced with

"#{localized_resource_name} was removed. <a ...>Click to undo.</a>"

Of course, we could always just skip all of this by adding a confirmation popup when you click "Remove", that also tells you to "Hit 'Cancel' to undo the changes." Great for simplicity, not as great for usability...

@dstosik, thanks for the work you've done on this, and I don't mean to be blocking this PR - I want it as much as you do. I've just been thinking about it for a while now. My biggest problem with the current implementation is that it's not immediately clear that undo is historical and incremental, and when you undo, it's hard to see exactly which one was brought back, and it's not clear that there's only one that was restored.

What if when something is removed, there's a trash menu that appears that will let you expand and restore the deleted items individually?

@igorbernstein @seanlinsley Any thoughts on this stuff?

@graysonwright

I think that replacing the removed fields with:

"#{localized_resource_name} was removed. <a ...>Click to undo.</a>"

is a much cleaner solution than creating a trash menu. It gets my 👍.

I don't think we'd need an option to expand deleted items before restoring them. If you want to see what was deleted, you could simply restore it then delete it again.

@seanlinsley
Active Admin member

@shekibobo and I are currently discussing this in IRC

@seanlinsley
Active Admin member

Action items:

  • make the JS sortable code not naively assume the largest index value is the current length
  • write example code for how people can normalize the index if they want, and add it to the docs

And then the UI question. Quoting myself from IRC:

Assuming we go the "X was removed. Click to undo" route, the only question left would be ordering. Particularly: what happens when you delete, reorder, then undo the deletion?

To solve that, it seems like they should still be part of the sortable UI. But this brings up a new problem: if there are many deleted records, it'd be hard to keep track of them e.g. if you want to restore just one.

To solve that, it seems like we should show e.g. the first 50 vertical pixels of the fieldset, except greyed out with a UI to restore it.

I haven't tried building a mock-up of what I have imagined, but I'd love everyone's thoughts.

@lukasz-wojcik

I know that a lot of time passed but is that going to be reviewed/merged any time?

@seanlinsley
Active Admin member

@sonar0007 if you would like to branch off of this PR to continue the work, I'd be happy to continue reviewing it.

@timoschilling
Active Admin member

Maybe some of this is already happen in #3862

@lukasz-wojcik

I will take a look at that

@timoschilling
Active Admin member

This PR should wait until #3862 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment