Adding a new content type, "default parent item" is shown even if no menu is selected

Hiding the menu when there is nothing to display is quite easy and have just updated the menu.admin.js to do this.

However, abstracting this to the form API is something that will require a little more work.

Adding a patch for the time being.

Bot liked it and it applied nicely to my local install. It's pretty simple JS, but might be good to have it reviewed especially in light of thinking about greater abstraction.

+ $select.parents('div').first().hide();

Shouldn't that be

+ $select.closest('div');

We might want to get more specific than that for the selector. like .form-item or something.

As far as abstracting that, not sure. But we could improve the speed of this thing by not using .append in the loop. It's not really good practice to begin with.

Updated patch with closest() and not parents().

I'm not sure what would be a better practice than using append()? From my reading, its not totally inefficient. Feedback appreciated.

can you put the $select.closest('div') in a variable before the if? That kind of selector duplication is a pain to refactor afterwards.

#8: drupal8-menu_option_updates-1829328-8.patch queued for re-testing.

This doesn't really belong to the field system.

#8: drupal8-menu_option_updates-1829328-8.patch queued for re-testing.

Now with added duplication avoidance!

This needs a JS review, but it's so very short. I've tested it in a Mac with FF, Safari, Chrome & Opera. Works as expected. I'm including a before/after shot.

I did have questions about the comments. The text will appear like this in the text:

       // Add new options to dropdown.
       // Keep a count of options for testing later.

But think this is more common.

       // Add new options to dropdown. Keep a count of options for testing 
       // later.

Heck, maybe it could even go on one line.... :)

I think this will offer usability improvements too! Looks good.

Screenshots as promised.

+++ b/core/modules/menu/menu.admin.jsundefined
@@ -33,13 +33,25 @@ Drupal.menuUpdateParentList = function () {
+      var parent_div = $select.closest('div')

missing ;

Also, you can use .toggle()

// Hide the parent options if there are no options for it.
$select.closest('div').toggle(totalOptions > 0);

instead of the if.

will re-roll today

Oh ya, I think it would be better if you also added the HTML5 hidden attribute as per:
http://www.paciellogroup.com/blog/2012/05/html5-accessibility-chops-hidd...

Both hidden and aria-hidden?

From the link

If you want to hide content from all users, use the HTML5 hidden attribute (along with CSS display:none for browsers that do not yet support hidden) There is no need to use aria-hidden.

Just using hidden for now.

@Christian Biggins - the recommendations I've seen suggest just using HTML5's hidden attribute. I think that provides all the semantic information & is well supported by AT.

HTML5 supersedes ARIA where they serve the same purpose.

#22: menu-hidden-1829328.22.patch queued for re-testing.

I'm trying to look at this test locally, but not sure which test to find this one under:
CollapsedDrupal\system\Tests\Upgrade\BlockUpgradePathTest

The problem is with Drupal\simpletest\WebTestBase->xpath('//li[@class=:class]', Array) called by simplexml_import_dom(): Invalid Nodetype to import

But I can't get to the full results to see what the test is failing over.

#22: menu-hidden-1829328.22.patch queued for re-testing.

Sorry for lack of response on this. Looks like its all ok?

Just tested the patch in #22 http://simplytest.me/project/drupal/8.x

It's working as expected.

Just to check on this bit:

        $select.append(
             $('<option ' + (machineName === selected ? ' selected="selected"' : '') + '></option>').val(machineName).text(options[machineName])
           );
+          totalOptions++;

It's not possible to test something on $select rather than keeping track of the totalOptions separately?

If I'm here - en/admin/structure/types/manage/article

And I go to the Console in Chrome, why do I get:

> console.log(machineName);
ReferenceError: machineName is not defined

I'm just trying to see what other options there are other than incrementing a value in the loop.

Great feature!!!
and it works properly as showed in the following picture

Putting this back to needs review as we should address @catch's comments about totalOptions.

Thanks so much for the review @solange_sari very useful.

#22: menu-hidden-1829328.22.patch queued for re-testing.

#22: menu-hidden-1829328.22.patch queued for re-testing.

from #31

Just to check on this bit:

$select.append(
$('').val(machineName).text(options[machineName])
);
+ totalOptions++;
It's not possible to test something on $select rather than keeping track of the totalOptions separately?

It seems fine to keep track of the options count this way, since the var is being used as a truthy test for the toggle and attr methods.

Ok, I'm going to set it back to RTBC then.

#22: menu-hidden-1829328.22.patch queued for re-testing.

Now we have jshint in core :)

+++ b/core/modules/menu/menu.admin.jsundefined
@@ -32,14 +32,19 @@ Drupal.menuUpdateParentList = function () {
+      $select.closest('div').toggle(totalOptions > 0).attr('hidden', totalOptions == 0);

Should be ===

Problem synopsis      JSHint: Expected '===' and instead saw '=='. (at line 47)

Nice

Committed 7ba4db0 and pushed to 8.x. Thanks!