Menu fast edit node links [#9178]

Comment	File	Size	Author
#18	menu_fast_edit_0_0.patch	2.78 KB	matt westgate

#16	menu_fast_edit_0.patch	3.17 KB	matt westgate

#14	menu_otf.patch	3.76 KB	matt westgate

#6	menu_quick_link_0.patch	4.12 KB	matt westgate

#2	menu_quick_link.patch	5.91 KB	matt westgate

	menu_fast_edit.patch	1.18 KB	matt westgate

Comment #1

JonBob CreditAttribution: JonBob commented 10 July 2004 at 17:21

This is interesting. I do have a couple reservations.

This adds a bunch of queries to node listing pages, since it has to check for every node whether that node has a menu item set.

I also wonder how much time this saves the user in reality. It saves navigating to the menu admin screen, sure, but provides little other help. It seems that if we are to provide a link to the menu item add page, we should at least be pre-filling in the path and title fields for the user. The provided method is little better than having the menu admin page open in another browser tab.

Maybe a node link isn't the right interface for this, anyway? Possibly a tab, or a hook_nodeapi('form admin') injection? If done right, this could finally replace that aging "link title" field in page.module nodes.

Log in or register to post comments

Comment #2

matt westgate CreditAttribution: matt westgate commented 24 January 2005 at 21:23

File	Size
menu_quick_link.patch	5.91 KB

This new patch gives users the ability to define menu items on any node creation/editing form. Screenshot here:

http://asitis.org/tmp/menu_quick_link_form.png

It does this through nodeapi (as suggested by Jonbob), which means that any node has access to these fields unless specified otherwise via the new menu configuration settings.

The functionality lives in menu.module, and users with 'administer menus' permission are allowed to edit the menu item fields inside the node form.

One more note. I'm also proposing the addition of a misc/drupal.js file to Drupal core (* gasp *). Currently it includes javascript functions to hide regions of a page. When a link is clicked on for a collapsed region, the region unfolds revealing the additional parts of a page. In this case the menu item fields are displayed after clicking the 'Menu item settings' link. I think it helps make best use of screen real estate, but we'll leave that verdict up to the usability experts.

Let's remove the js file if it's the only thing stopping this patch from being submitted. I thought it might be worth exploring, especially for form pre and form post regions of the node form.

Log in or register to post comments

Comment #3

matt westgate CreditAttribution: matt westgate commented 24 January 2005 at 21:34

Component:

other

» menu system

Woops. Setting status to 'patch'.

Log in or register to post comments

Comment #4

Bèr Kessels CreditAttribution: Bèr Kessels commented 25 January 2005 at 13:17

+1 on the feature, I use menu_otf all the time.
-1 for the javascript. Not that i am against it, ion contrary. I think we should not start implementing JS on a per-case basis, but we should introduce a javascript system that can do your accordeon-magick on any element.
So it should wait IMO.

Log in or register to post comments

Comment #5

Dries CreditAttribution: Dries commented 25 January 2005 at 15:07

I agree with Ber: +1 for the feature, -1 for the Javascript. This would (or should) be easy to implement/integrate as soon the new node forms are done. Both the new node forms, and this feature are crucial.

Log in or register to post comments

Comment #6

matt westgate CreditAttribution: matt westgate commented 27 January 2005 at 20:39

File	Size
menu_quick_link_0.patch	4.12 KB

Okay, here's the latest patch, footloose and javascript free.

Log in or register to post comments

Comment #7

robertDouglass CreditAttribution: robertDouglass commented 1 February 2005 at 08:04

+1 to the feature, with or without javascript.

The Menu item title:* is showing that it is required, but it can't be required as that would force users to create menu items (and it doesn't validate to be required).

The patch does, however, open up a big can of permissions worms, and lacks configuration. I would like to see a checkbox on the Workflow (/admin/node/configure/types/{type}) page so that I can hide this on a per-type basis.

Then, I think we need some way of limiting this by role, by user, and by menu. I would at least want an "excluded menus" list where I could, say turn off the main Navigation menu so that people don't go and trash it.

Very cool feature in a very important direction! I sincerely hope this makes it into the code freeze.

Log in or register to post comments

Comment #8

(not verified) CreditAttribution: commented 2 February 2005 at 22:37

I removed the required mark next to menu item form field. Thanks for spotting that.

I don't completely understand what you mean by this patch 'opening up a big can of permissions worms'. User's with 'administer menus' permission are simply given an easier way to add menu items while creating content. I think it's better to keep it simple to begin with.

http://asitis.org/tmp/menu_quick_link_form.png

There actually is a configuration interface (settings/menu). Your menu cache probably needed to be cleared after applying the patch :-) On that page you can choose which node types acquire the quick menu item form field as well as define the default menu item.

I disagree with putting the menu item form visibility settings on the admin/node/configure/types/{type} page. Those settings are for the default state of the node instance, not what's visible on the form field. For example, I wouldn't want to see a list of all 'form pre' and 'form post' fields on that page to select from, but it could be useful as a standard interface elsewhere.

Log in or register to post comments

Comment #9

matt westgate CreditAttribution: matt westgate commented 2 February 2005 at 22:40

Last comment by 'learning to login' mathias.

Log in or register to post comments

Comment #10

Dries CreditAttribution: Dries commented 6 February 2005 at 10:23

I'm going to put this on hold until the collapsbile form groups settled. I encourage you to participate in the design and discussion so that it meets your requirements. The same should be done for other parts of the node edit form, so let's try to come up with a generic solution that can be used through an API that shields you from all Javascript details.

Log in or register to post comments

Comment #11

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 13 March 2005 at 22:13

Hmm, should we have a status "on hold"? I am unsure what to do with this one. It still applies.

Log in or register to post comments

Comment #12

TDobes CreditAttribution: TDobes commented 29 March 2005 at 10:15

+1 on this... trying to explain to users that they need to add their pages, then add their pages to the menu system (and explain why) was incredibly difficult... menu_otf has helped with this, and I think this sort of functionality really belongs in core.

Log in or register to post comments

Comment #13

Dries CreditAttribution: Dries commented 29 March 2005 at 14:05

Yes, we need something like this in core but as explained, there is a depency on the new node form (and collapsible form/page elements in particular). Is anyone going to work on this? I'm determined to help get this in core ASAP but can't take a lead in this. Takers?

Log in or register to post comments

Comment #14

matt westgate CreditAttribution: matt westgate commented 24 June 2005 at 21:41

Priority:

Minor

» Normal

File	Size
menu_otf.patch	3.76 KB

Now that collapsible form/page elements are in core it's time to revisit this patch. Attached is the latest version pulled straight from the menu_otf module. Here's a screenshot of it in action:

http://asitis.org/images/motf/motf-add-item.png

Log in or register to post comments

Comment #15

Dries CreditAttribution: Dries commented 27 June 2005 at 19:01

1. Use quotes around strings when indexing an array. It generates warnings. See menu_nodeapi(). AFAIK, the $a3 and $a4 style parameters are deprecated. It's no longer necessary to specify these as module_invoke_all() and friends got fixed.

2. The form descriptions talk about a 'quick item form'. I have no idea what that means, or what it refers to.

3. I'm not too happy with the global 'Default parent item' setting. Is this really necessary? Less settings is better.

4. t('Menu item deleted.') We updated the status messages a couple weeks ago. Messages like these have been reworded for clarity and to use the proper tone. Read up on the old issue for details, or poll either Stefan or occy. (I asked them to write some guidelines, not sure they did. I can't seem to find any though.)

5. There is an ul { at the bottom of the patch?

6. The GUI seems to mix 'menu item' and 'link'. This might be potentially confusing.

Log in or register to post comments

Comment #16

matt westgate CreditAttribution: matt westgate commented 1 July 2005 at 05:04

File	Size
menu_fast_edit_0.patch	3.17 KB

Patch has been updated per Dries' suggestions.

Log in or register to post comments

Comment #17

Dries CreditAttribution: Dries commented 2 July 2005 at 12:18

One more review:

- The parent selection menu doesn't seem to get populated. All it says is 'Navigation', but no existing menus are shown. I'm not sure it is a bug, but it confused the hell out of me. Intuitively, I'd think I'm able to add a menu item to any existing URL/path know to the menu system ... not?

- Do we need all these hidden variables? I think it would be better to make menu_edit_item_save() smarter so it only saves the values that are set. That saves about 10 lines of code.

- The title field is marked required?

Log in or register to post comments

Comment #18

matt westgate CreditAttribution: matt westgate commented 3 July 2005 at 17:32

Status:

Needs review

» Active

File	Size
menu_fast_edit_0_0.patch	2.78 KB

- The parent selection menu is fixed now and works as expected.

- I didn't touch menu_edit_item_save() since we would have also gained at least 10 lines of code determining which fields are set and creating column and value arrays, etc. But I did eliminate the array construction I had in place before passing off to that function, removing 6 lines of code.

Log in or register to post comments

Comment #19

m3avrck CreditAttribution: m3avrck commented 18 July 2005 at 15:51

This patch is excellent!!! Just tried it out on the latest CVS version and this is greatly simplifes the workflow when creating simple, e-brochure type websites. Get this committed for the next release, two thumbs up! :-)