Drag and drop enable book outline [#192736]

The book outline (content/book/x) has now been updated for drag and drop also. It's glorious.

This patch is dependent on http://drupal.org/node/181126, which makes a few minor tweaks to the drag and drop javascript. It'll work without it, but the other patch will correct a few odd behaviors.

The only significant change required in book.module was not listing the book itself in the outline view. Exposing the book item itself as the first item in the list would erroneously let the user drag an item outside of the book by moving it all the way to the left. I don't think this is at all undesirable though. Listing the book itself in the outline is similar to having the title of a (real paper) book as Chapter 1 in the table of contents then everything underneath it as a sub-chapter. From an outline sense, it's not logical to include the book itself as the top item.

Comment	File	Size	Author
#14	book_drag_and_drop.patch	16.27 KB	quicksketch
#14
#4	book_drag_and_drop.patch	10.74 KB	quicksketch
#4
#3	book_drag_and_drop.patch	10.2 KB	quicksketch
#3
#2	book_drag_and_drop.patch	7.84 KB	quicksketch
#2
	book_drag_and_drop.patch	7.86 KB	quicksketch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 16 November 2007 at 11:55

Status:

Needs review

» Needs work

Again this looks great. I agree with the changes to the book title listing, didn't miss it at all.

Same browsers, same issues as #25 on the block drag and drop, so CNW unfortunately but these must be pretty minor.

Comment #2

quicksketch

he/him

CreditAttribution: quicksketch commented 17 November 2007 at 01:05

File	Size
book_drag_and_drop.patch	7.84 KB

I'm going to need help on this issue for sure. If pwolanin or chx could look over the code that is saving menu items in book.module I would greatly appreciate it.

Here's the problem. Drag and drop is operating fine, but book.module is not setup to handle mass-reparenting of items. Frequently when arranging a small book, the parenting behavior will work fine on the page, but when I save the changes, several of the items disappear from the book.

Here's a video of this nasty behavior http://quicksketch.org/sites/quicksketch.org/files/book_eating_items.mov

To help reproduce you can download this D6 dump: http://quicksketch.org/sites/quicksketch.org/files/drupalhead.sql
user: admin
pass: admin

Follow the same steps as in the video and you'll reproduce. Here's an updated patch that is the code I'm currently using.

Comment #3

quicksketch

he/him

CreditAttribution: quicksketch commented 17 November 2007 at 06:33

Status:

Needs work

» Needs review

File	Size
book_drag_and_drop.patch	10.2 KB

Ahhh. I found the cause for the weirdness. I explain it more in-depth at the menu issue, but I'll post the solution here also.

Basically, we can't save children in the hierarchy before their parents. Because we had been saving them in the order set in the $form array, often times children were being saved before their parents. To fix this I'm now saving the elements according to the new order on the page, by checking the order passed in by the POST array. Note I'm not actually using any post values, just checking the order in which they come in.

  // Save elements in the same order as defined in post rather than the form.
  // This ensures parents are updated before their children, preventing orphans.
  $order = array_flip(array_keys($form['#post']['table']));
  $form['table'] = array_merge($order, $form['table']);

After applying #34 from the menu patch, this patch works great!

One more change of significance: we no longer do a node_load/save for each item moved in the tree. This is only done if giving an item a new title. This speeds things up quite a bit when rearranging a large number of items all at once.

Comment #4

quicksketch

he/him

CreditAttribution: quicksketch commented 17 November 2007 at 14:24

File	Size
book_drag_and_drop.patch	10.74 KB

This patch adds a little more documentation and references the very similar menu_overview_form_submit() function. The parent item field is now hidden again.

Comment #5

pwolanin CreditAttribution: pwolanin commented 17 November 2007 at 18:36

subscribing to remind myself to review

Comment #6

BioALIEN CreditAttribution: BioALIEN commented 18 November 2007 at 02:26

quicksketch just keeps on sketching. subscribing!

Comment #7

catch

he/him

English

CreditAttribution: catch commented 20 November 2007 at 15:36

Status:

Needs review

» Needs work

I don't get any indentation in FF2 or IE7. Everything else smooth and snappy but CNW.

Comment #8

webernet CreditAttribution: webernet commented 20 November 2007 at 15:55

Garland:
Indentation looks fine up to level Four - then the text box shifts to the line below the drag icon.

Bluemarine:
Arrow is indented properly, but text box is shifted to the next line for all levels.

(FF2/WinXP)

Comment #9

webernet CreditAttribution: webernet commented 20 November 2007 at 16:04

It also appears that it's allowing me to attempt to move a page to depth 10 (> than Max_depth). The error that should be appearing appears to be suppressed.

I also somehow managed to lose the titles and depths of a bunch of items...

While doing further testing - it appears that the indentation is now working under Firefox on bluemarine after making some changes...

Comment #10

quicksketch

he/him

CreditAttribution: quicksketch commented 20 November 2007 at 18:22

What do we think about removing the text box entirely for titles? The width of the text box causes wrapping when it's not necessary when the title is short, and for really long titles cuts off the title. Just a thought.

Fortunately we're not going to have to deal with indentation not showing up any more now that the menu patch has gone in (which this patch was dependent on). Without the new theme_indentation() function, no indents would show up. Now that it's in reviewers won't be having this problem any more.

Comment #11

catch

he/him

English

CreditAttribution: catch commented 20 November 2007 at 20:20

Removing the textfield for titles seems like a good idea. Apologies for #7, was in a hurry and forgot the menu patch. I now get the indentation, but I'm losing it on save (using the tarball from mid-day though and have the same issue with menus - is there another dependency?).

Comment #12

pwolanin CreditAttribution: pwolanin commented 21 November 2007 at 22:51

The ability to change titles seems like a key feature of this admin interface - I think it would be a regression to remove this. Is it just a visual problem? Is there some way with the JS to make the the title appear normal and then switch to presenting the textbox when clicked?

Comment #13

techczech CreditAttribution: techczech commented 23 November 2007 at 18:53

Applying the patch in #4 works fine with the same layout breaking problems in Garland as were mentioned above. Only the first two levels are shown as intended at 800px and 4 at 1080px width of window. I'd hate to see the ability edit titles inline go but the drag and drop functionality seems to be more important. One possible solution might be to apply less indentation to differentiate levels. The suggestion by pwolanin for inline editing would be ideal but not worth it if it would hold up other things. I remember that inline editing was one of the items on Dries' agenda for D7, though. It would be great to see it across the board but right now the Book Outline manager is the only place it exists so removing it until a more systemic solution can be found would probably not be such a great loss.

Comment #14

quicksketch

he/him

CreditAttribution: quicksketch commented 26 November 2007 at 02:16

Status:

Needs work

» Needs review

File	Size
book_drag_and_drop.patch	16.27 KB

Well after tinkering a bit I think I'd agree that saving the ability to rename titles is desirable, and if it's easy enough to keep then we shouldn't take it out. This patch takes a very simple approach and sets the table width to 56em, preventing any wrapping at up to 7 parents (the maximum amount).

It also makes the maximum depth a setting passed into tabledrag.js. The menu outline is set to 8 parents max, which books get set to 7, since the book itself is a parent that's not shown on the page.

Comment #15

catch

he/him

English

CreditAttribution: catch commented 26 November 2007 at 09:59

Flawless with IE7/FF2/Safari3/IE6 on XP. Also tried w/o javascript on a couple of browsers and it degrades cleanly. Should be RTBC but could do with a review on another platform.

Comment #16

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented 26 November 2007 at 10:14

Status:

Needs review

» Reviewed & tested by the community

This works perfect on MAC OS X in all possible browsers. Great work!
This is definatly RTBC...

One question though, Nate why do we still display the "Weight" columns in the JS-versions?

Comment #17

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 26 November 2007 at 13:12

Indeed, why do we display the weight? (This is understandable with the input format dragger for example, as there is some information in the default weights). Why is it displayed here?

Comment #18

quicksketch

he/him

CreditAttribution: quicksketch commented 26 November 2007 at 15:37

The weight field *shouldn't* be visible, that's strange. I double checked with a brand new Drupal install and checked in Opera 9, Safari 3, and Firefox 2 on Mac. I don't have my Windows hard drives available to check those, but with no JS changes that affect those areas, there shouldn't be browser differences.

Comment #19

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented 26 November 2007 at 15:58

Not sure wha went wrong, but I also did a new install of drupal and applied the patch. The weight column isn't visible..
No code remarks at all...

Let's get this in, Goba/Dries?

Comment #20

Dries CreditAttribution: Dries commented 26 November 2007 at 16:19

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD. Thanks a bunch guys! :)

Comment #21

quicksketch

he/him

CreditAttribution: quicksketch commented 26 November 2007 at 16:37

Status:

Fixed

» Reviewed & tested by the community

Woot! Taxonomy stands alone :)

http://drupal.org/node/193333

Comment #22

quicksketch

he/him

CreditAttribution: quicksketch commented 26 November 2007 at 16:38

Status:

Reviewed & tested by the community

» Fixed

Comment #23

techczech CreditAttribution: techczech commented 26 November 2007 at 23:22

I'm still finding that the 'Operations' columns slide under the right sidebar with any horizontal resolution under 1160px. Removing the 100% width from the id 'book-outline' fixes this but reintroduces the problem with wrapping. Am I doing something wrong?

Comment #24

(not verified) CreditAttribution: commented 10 December 2007 at 23:23

Status:

Fixed

» Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Comment #25

techczech CreditAttribution: techczech commented 12 January 2008 at 11:46

Status:

Closed (fixed)

» Needs review

The drag and drop layout of the book outline breaks Gardland in default install of RC2, again. I thought the patch in #14 fixed this but it doesn't seem to have made it into RC2. In my view, this is close to a critical error, since the layout is broken 'out of the box' with the default theme.