Use on/off checkbox to enable node's menu item

Can you please supply a clear screenshot with the problem, and the proposed solution? I am kind of torn between all you'r questions, assumptions and notions that I can't and hopefully others as well, what your proposing. You'r title is quite confusing, I tried to reword it but probally failed.

I am looking foward to a response, as it seems you encounterd this a lot of times.

Automatically populating the menu item title with the title of the node is a great idea; it's something that I've often wished for myself, but failed to create an issue or patch. Thank you!

Your suggested logic regarding the check box is a substantial improvement as well. So far, this sounds like a good plan.

Aaaah, much better. I suspected it was about something like this but wasn't sure either :) Good presentation and a fine proposal me thinks. Using a checkbox for a delete action is indeed strange. And your proposed new workflow looks like it would be a lot more intuitive. Probably needs some more discussing the technicalities, can't help there but: Good Idea.

It's a fine idea and would certainly be an improvement.

Going by the suggested approach, the menu settings (title, description and parent) would be redundant as long as the checkbox is unchecked. Some users may fill out the menu settings while failing to check the checkbox.

Ahh, after the mockups has been placed I totally get it, sounds good. As yoroy said, we need some reviews.

dman: These are all real cool ideas. If you are up to quickly prototyping workable demos of these variations that would rule. Doesn't even have to be functional, but just demonstrate the flow. It's hard to base this on just thoughts, we'd want to user experience it :)

I will reference this request in #483078: Menu item name should be node title by default. I think both these requests are complimentary:

• The menu feature's current implementation is like a light where you have to use one switch to turn it on and another to turn it off. This request is an excellent idea because it converts this to one switch.
• #483078: Menu item name should be node title by default suggests recasting the menu title field as a "put text here if you don't want to use the node title." If you leave it blank, the menu name is the same as the node title--which I expect would happen in the vast majority of cases anyway.

I also recommend #576916: Default menu "Parent item" should be Navigation, not Primary links so that new links default to Navigation instead of Primary links. Primary links are space-limited and need to be minimal to be effective. Navigation is the workhorse and should be where links go unless otherwise specified.

The confluence of all three issues is a really easy way to add a menu item for the node: just check a box, and it "just works". With this, you might even hide all the menu details by default, assuming that the vast majority of links will be fine with these default settings.

Simplifying title and filing in the proper component.

I think that this could be handled by jQuery alone. When jQuery is disabled, the functionality remains the same. What should jQuery do:

menu.js

- Create the checkbox and hide all other fields
- Add a listener to this checkbox, onChange = function(){
  if(checked){
    - show all other fields
    - $('edit-menu-link-title').text() = $('edit-title').text();
  }else{
    - hide all other fields
    - $('edit-menu-link-title').text() = '';
  }
}

I'll have a look!

This should do the trick, please test.

- When checked, it copies the value of the page title to the menu title field.
- It is always possible to override the menu title.
- When the menu title is cleared, the checkbox will be unchecked as well.

I haven't reviewed the code yet but I tested it, and I like what this patch does.

I think this provides us an opportunity to provide some better text. Instead of 'Create new menu item' maybe we could say something like 'Add a link to this post from the site navigation' or something.

- Changed tabstops to spaces
- Changed the text to Add a link to this post from the site navigation, I agree with Dries on this.

BarisW: I like where you're going, but be careful about changing the menu name field only with Javascript. That's why #410464: Menu name = node title upon node edit through user interface failed. See Dries's comment in #48 and mine in #50. That feature could initially populate the menu name upon menu item creation but couldn't keep the menu name field in sync with future node title changes. Also, being Javascript-only, no fix could keep the menu name in sync after API-based node edits.

A complete solution needs PHP changes and probably needs to use the node title for the menu name if the menu name field is blank. See #483078: Menu item name should be node title by default.

@dman: Is it obvious to novices? What will they think when the menu name doesn't change after node title changes?

I still maintain that the best design should start with "blank menu name = use node title." If menu logic is well-encapsulated, then the change ought to be achievable for 7.x.

Does Javascript prepopulation takes Drupal further from this? Is that good?

For me it´s very clear that the menu title could differ from the node title (eg: Contact form as Node title and Contact as Menu title).
When a user checks the checkbox, the menu name = node title. This is always overrideable (is that a word?).

When the menu field already contains text, the node title will not be copied into it. So indeed, if a user changes the node title afterwards, the menu title remains. And I think this is a good idea (and works the same as the Path settings, these also remain the same after a node title change).

I agree with @dman: this is just added value and doesn't need any PHP. It just makes editing quicker and easier for end-users. And it is degradeable to the current functionality when the users doesn't have Javascript enabled.

+++ modules/menu/menu.js    3 Oct 2009 12:50:08 -0000
@@ -10,4 +10,23 @@
+      if($(this).is(':checked')){

Coding style: needs space between ) and {.

+++ modules/menu/menu.js    3 Oct 2009 12:50:08 -0000
@@ -10,4 +10,23 @@
+      }else{

Coding style: else statement should be on different line.

I'm on crack. Are you, too?

Tnx for the tips, I'll read the coding style guide :)
Attached a new version.

- Added a newline and a space :p

Tnx for the tips, I'll read the coding style guide :)
Attached a new version.

- Added a newline and a space :p

Something is wrong with most recent patch. The link in invalid. Test bot can't can't test it. Please rename and re-upload.

I think I pressed the upload button twice, which also removed the tags.
Re-added them and uploaded the patch again.

Again...

for bot

Guys, we need some testers to get this RBTC.
Please help :)

This got committed by accident and is horribly broken. I searched for a couple of hours for the actual issue.

We've got a process issue here: how to work on something that was committed by accident?

I opened a related issue yesterday which @sun marked as a duplicate of this. Not a 100% sure that is the case: #620314. My complaint is that the node/xxx/edit form does not display saved settings for the menu link title, parent, and weight. in a pretty consistent way in Drupal, node/add and node/xxx/edit are the same form and node/xxx/edit retrieves the current value for all the fields. In D6, the node/xxx/edit form does indeed show you the current value of the menu link title, parent and weight.

It is not clear to me from this thread whether the current values for the menu link (title, parent, weight) were supposed to be retrieved or not. I do hope it was the intention of the new functionality to retrieve the values of the existing setting for node/xxx/edit, which is consistently the case for Drupal forms.

While this situation here is messy, I'm going to toss out two related things for reference. I do this not to confuse the matter, but to try to get related issues on the table.

1. In D5, the anchor title attribute field, labeled "description" was also available on the form; that was removed for D6. In D6, the node title populates the anchor title attribute in the background. I think it is fine to make the form shorter and easier to use. But there is a growing consensus within the usability and accessibility communities now that the title attribute of an anchor tag should not be used unless there is good reason to use it. Drupal should default to leaving it empty. It can be added via the menu admin interface if it is needed. I was intending to file an issue on this. But I'll wait till the dust settles on the current related issues.
2. The anchor title attribute field, labeled "Description" on the menu admin interface is broken. It is saving to the db but not being spit out in the html. I reported that yesterday at: http://drupal.org/node/620324

It seems that someone (@Dries?, @Webchick?) needs to step in and provide some order here about how to sort all this out.

Shai
Content2zero

This is what you want, I think. Works nicely for me.

Has only one minor race condition: The vertical tabs fieldset summary is not directly updated when toggling the checkbox.

uhm. We have some more work to do here.

@sun, great job. I tested the patch. It applied cleanly and worked as expected. Great noticing the awful help text. My attached mock-up has alternative text. I guess for usability and search I'll repeat text from the graphic here.

• Proposed help text for Menu link title: The menu link title will be the clickable text for the menu item.
• "Parent Item" drop down should changed to "Parent Menu"
• Proposed help text for Parent Menu: Menus are enabled by content-type. Go to content-type settings and click on Menu Settings tab to enable a menu for this content-type [It would be nice to have a hyperlink here directly to the settings for the appropriate content-type... Can you have hyperlinks that will open a field set?]
• Proposed help text for weight: menu items with a higher number appear lower in the list.

Weird behavior I'm seeing on my install: In Garland, primary nav items have anchor title attribute set to be = $node -> title. Menus items in the left sidebar have no title attribute set. Are other people seeing that?

I feel we are moving from chaos to order!

Re: @sun in #34. Not sure programmatically what the reasons are for the two boxes or what the upshot of changing two check boxes to one would be.

But assuming we need to have two boxes... if the description text could say "Create menu link" before anything has ever been saved there and switch to "View menu link settings" after it had been saved once... I think we would remove a lot of the confusion. Would that contextually conditional display text be hard to do?

Shai

Thanks, Shai.

Proposed help text for Menu link title: The menu link title will be the clickable text for the menu item.

Honestly, I think that this info is superfluous. By renaming the label to "Link text", we can drop the entire description.

"Parent Item" drop down should changed to "Parent Menu"

Unfortunately, the select list contains both menus AND links. As long as that is the case, I think that "Parent item" is the most precise thing we can call it.

Proposed help text for Parent Menu: Menus are enabled by content-type. Go to content-type settings and click on Menu Settings tab to enable a menu for this content-type [It would be nice to have a hyperlink here directly to the settings for the appropriate content-type... Can you have hyperlinks that will open a field set?]

That entire help text is for site administrators only, and many users won't even have permissions to alter the content-type settings.

Since the current description is plain nuts, I think we can just drop this entire description as well.

Proposed help text for weight: menu items with a higher number appear lower in the list.

For now, I'd just drop the "Optional." Easily explaining how the weighing works to someone who mainly doesn't care is a really tough call. In reality, the entire weight widget makes no sense and is the wrong interface element, but that is to be discussed and resolved in other issues. See #491022: Remove weight field from menu item entity forms and #605894: Replace menu weight selector on node/add with relative item positioning.

And re: #36:

Yes, we can merge those checkboxes into one, which will also simplify the JS of this patch a bit, because it currently takes special care for not submitting a link title when the checkbox is disabled.

So here we go.

This may sound funny, but I really hope that at least some tests will fail. Otherwise, this entire node menu widget does not have any tests.

@sun, sorry that your patch didn't fail :)

I reverted to the latest dev version of 7 and then applied the patch in #39. It applied cleanly. It seems to be behaving as designed. This is sooo much better. One caveat is that I'm watching the world series so I'm a bit distracted. But I'm a Phillies fan so I'm having fun :).

@sun, can you comment on:

• Whether you think the node UI should apply an anchor title attribute
• Are you seeing the behavior that I am: a menu item in the sidebar outputs no anchor title attribute but the primary nav does output an anchor title attribute, outputting the $node -> title.

Shai

Well, then we also need to add tests here.

Shai:

1) That's not related to this issue/patch. See #378064: Description field missing in menu settings on node form.

2) Equally not, but not sure whether there's an issue for that.

Until there are tests...

.

.

uhm, was that a total sign-off from the UX team? ;) (please note the changed labels and vanished descriptions in the before/after screenshots above :)

@tha_sun We tried to be equally cryptic to your drutastic image :P. The "please note" really helps, although I am unsure if you now fixed the issue in #34 of having two checkboxes.

I think the descriptions removal is good, although the label change on "Link text" should be reverted to "Menu link title". That label tested well, no reason to change something that tests well.

@Bojhan, the latest patch from #39 does indeed fix the two box problem pointed out in #34.

Shai

The direct comparison is before and after.

1) There is only one checkbox "Provide a menu link".

2) The label "Menu link title" was renamed to "Link text", because half of the lengthy form element description tried to explain that this "title" will be used as "link text". "link text" is the proper, standards conforming term, and using this term will additionally make a patch for #378064: Description field missing in menu settings on node form easier, because that one would expose a "link title" or "link description" form element, which is visible for screen-readers, and when hovering a menu link.

3) The entire description for "Parent item" (the select contains both menus and menu links) was removed, because it tried to explain a in-depth technical detail of the menu system, which is the entirely wrong place to explain it.

4) "Optional." in the description for the "Weight" select has been removed, because the selection contains no optional value. (All links must have a weight.)

The description under Weight needs simplification. It's wordy and inconsistently describes positioning: "sink" vs. "positioned nearer the top."

Suggestion: Menu links with smaller weights sort before links with larger weights.

11 words vs 18 words

Now including tests.

Also changed the label back to "Menu link title" after discussion with Bojhan.

Also changed the description for "Weight" based on Aren Cambre's proposal, but slightly modified.

Looks good, setting it to RTBC - time to fix this bug.

@sun @Bojhan,

• I deleted D7 code and did fresh install from Head
• Noticed the broken node/add menu setting behavior
• Applied patch from #50
• Applied cleanly
• I added and deleted menu items to nodes, new and already existing
• Everything good, except one outstanding issue: The status text under the Menu Settings vertical tab requires a mouse click (in no particular location) to update itself with the latest setting when there is a change. Not horrible, but it would be super slick if it updated without needing a click.

This is lookin' really good.

best,

Shai

ok, this one additionally fixes the vertical tab summary updating.

No other change.

let's have test bot do a double check :-)

I applied the patch in #53 to a fresh D7 HEAD. All three files applied cleanly. I tested on new nodes and on existing nodes. Everything is working, including the new change so that the Menu title link status in the "Menu Settings" verticle tab updates instantly when it is change on the right.

Perfect.

Wow, this patch is AWESOME! Now. All we need is a follow-up patch to provide an option to set the default value of this checkbox at admin/structure/types and set that to TRUE on Page and we'll kill the #1 WTF in all of Drupal! :D

Oh-so-gladly committed to HEAD!

Wonder if this is worth a changelog entry, along with the ability to specify which menus are allowed to be selected from?

weirdness when changing the node title:

• fill in a node title
• tick the checkbox
• change the node title

Result: The link title is updated, but the vertical tab title isn't.

Solution: Trigger 'formUpdated' after changing the value?

oh, I apparently did not look into form.js. Nice one! So let's also fix the other instance. ;)

I tested #54. It applied cleanly to a fresh install of D7. Didn't break anything and provided fix as described. Note that I did the following test, which it passed:

* I made the Menu Link Title different from the node Title
* I then went and changed the node Title and this had NO effect on the Menu Link Text or the Menu Settings vertical tab information. (Which is good, right. If the user has customized the menu link title, we don't want editing of the Title to hijack the customized title.)
* Then I changed the Menu Link Title back to be exactly the same as the node title. I then changed the node title... and both the Menu Settings and the Menu Link Title responded and mirrored the title as I changed it.

Great work.

Re: #59. I at first thought that #59 would be cumulative, including the changes in #54. But when I applied #59 to a fresh D7 install, the changes described in #54 didn't happen. That's when I went back to a fresh install and applied #54. I'm not sure what #59 does, so even if I applied over the #54 patch, the only thing I would be able to report is if it applied cleanly and whether it appeared to break anything. I think I'll wait till I hear back from @sun regarding what new is in there.

Shai

Note that when I wrote #54 that was from the patch title; it referred to @seutje's patch from message #58.

@seutje, if you were following @sun's convention you would have labeled the patch with a #58. @sun's clever convention uses the comment number; it doesn't attempt to create a sequential version number which is what you must have thought, "since the previous one was 53, then I'll name this 54."

Shai

uhm - so #59 didn't work? Did you clear your browser's cache?

@sun. Correct, it didn't work; it didn't do the changes described in the patch in comment #58. It applied cleanly. But no, I didn't clear my browser cache (but hadn't for any of these other tests that worked). I'll try again.

But to be clear, please confirm that #59 is cumulative. In other words, you added changes to comment #58 patch. And so I should apply #59 to a fresh HEAD install and see the changes described in comment #58.

Also, could you note what changes you made in #59.

Shai

@sun: Oh yea, that makes sense. How did I not see that, it's only 5 lines higher...

@Shai : I'm sorry, I was unfamiliar with sun's convention or this issue. I just traced back the commit that broke my other patch and wanted to avoid naming collision.

Yes, #59 is cumulative, it applies to a clean HEAD. All sun did was replace some traversing and a low-level event trigger with a high-level event trigger that didn't require any traversing. Less code and more global, and therefore more flexible I'd say. It allows anything else on the page to easily pick up on the change and act upon it.

Applying #59 and performing a hard refresh (ctrl+F5 in FF) made me unable to break expected behaviour.

@seutje @sun,

I applied #59 to a fresh Drupal install. It applied cleanly

All of the basics are working:

But it doesn't pass that test I created in #60 on #54 (which did pass for #54). In that test, the title field was able to regain control of the Menu link title (and mirrored in the Settings Tab) when the Title and menu link were made to be in synch again after the menu link title had been previously customized. What not passing means is that once the Menu Link Title has been customized, the Title field can never gain control of the menu link title again.

I actually don't think that is a problem. I thought it was a cool bonus when it worked, but not really important if it's harder to do with the cleaner coding.

Shai

ok, what remains is a bug, but that's not critical. We should fix it though ;)

If we just target the id names in the menu.js? Have attached a patch, which seems to work ok.

We need to merge in #59 into #69.

Ok, merged 59 & 69 changes into a patch (attached). Seems to work ok at my end. Thanks.

Both patches seem to have been tested, so let's get this sucker done! :)

Committed to CVS HEAD. Thanks!