Skip to content

Commit

Permalink
fix(menu): type checkbox should not affect a normal menu item.
Browse files Browse the repository at this point in the history
* Currently we always wrap the menu-item content inside of a button, if the menu-item has an attribute with `type[checkbox|radio]`.
  This logic was originally designed for the `md-menu-bar` component, and not for the normal `md-menu`.

This caused confusion for developers and should be removed.

* Also removed a part of code, which was a duplicate and even not reachable, because the same code returned / exited above.

Fixes angular#8110
  • Loading branch information
devversion committed Apr 19, 2016
1 parent 7c6ff36 commit 3780e98
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 14 deletions.
7 changes: 0 additions & 7 deletions src/components/menu/js/menuServiceProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,13 +318,6 @@ function MenuProvider($$interimElementProvider) {
}
}

opts.menuContentEl[0].addEventListener('click', captureClickListener, true);

return function cleanupInteraction() {
element.removeClass('_md-clickable');
opts.menuContentEl.off('keydown');
opts.menuContentEl[0].removeEventListener('click', captureClickListener, true);
};
}
}

Expand Down
12 changes: 9 additions & 3 deletions src/components/menuBar/js/menuItemDirective.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ angular
.directive('mdMenuItem', MenuItemDirective);

/* @ngInject */
function MenuItemDirective() {
function MenuItemDirective($mdUtil) {
return {
require: ['mdMenuItem', '?ngModel'],
priority: 210, // ensure that our post link runs after ngAria
compile: function(templateEl, templateAttrs) {
if (templateAttrs.type == 'checkbox' || templateAttrs.type == 'radio') {

// Note: This allows us to show the `check` icon for the md-menu-bar items.
if (isInsideMenuBar() && (templateAttrs.type == 'checkbox' || templateAttrs.type == 'radio')) {
var text = templateEl[0].textContent;
var buttonEl = angular.element('<md-button type="button"></md-button>');
buttonEl.html(text);
Expand All @@ -24,7 +26,7 @@ function MenuItemDirective() {
angular.forEach(['ng-disabled'], moveAttrToButton);

} else {
setDefault('role', 'menuitem', templateEl[0].querySelector('md-button,button,a'));
setDefault('role', 'menuitem', templateEl[0].querySelector('md-button, button, a'));
}


Expand All @@ -51,6 +53,10 @@ function MenuItemDirective() {
templateEl[0].removeAttribute(attr);
}
}

function isInsideMenuBar() {
return !!$mdUtil.getClosest(templateEl, 'md-menu-bar', true);
}
},
controller: 'MenuItemController'
};
Expand Down
22 changes: 18 additions & 4 deletions src/components/menuBar/menu-bar.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,18 @@ describe('material.components.menuBar', function() {
function setup(attrs) {
attrs = attrs || '';

var template = '<md-menu-item type="checkbox" ' + attrs + '>Test Item</md-menu-item>'
var template = '<md-menu-item type="checkbox" ' + attrs + '>Test Item</md-menu-item>';

var checkboxMenuItem;
inject(function($compile, $rootScope) {
checkboxMenuItem = $compile(template)($rootScope);
// We need to have a `md-menu-bar` as a parent of our menu item, because the menu-item
// is only wrapping and indenting the content if it's inside of a menu bar.
var menuBarMock = angular.element('<md-menu-bar>');
var itemEl = angular.element(template);

menuBarMock.append(itemEl);
checkboxMenuItem = $compile(itemEl)($rootScope);

$rootScope.$digest();
});
return checkboxMenuItem;
Expand Down Expand Up @@ -398,11 +405,18 @@ describe('material.components.menuBar', function() {
function setup(attrs) {
attrs = attrs || '';

var template = '<md-menu-item type="radio" ' + attrs + '>Test Item</md-menu-item>'
var template = '<md-menu-item type="radio" ' + attrs + '>Test Item</md-menu-item>';

var radioMenuItem;
inject(function($compile, $rootScope) {
radioMenuItem = $compile(template)($rootScope);
// We need to have a `md-menu-bar` as a parent of our menu item, because the menu-item
// is only wrapping and indenting the content if it's inside of a menu bar.
var menuBarMock = angular.element('<md-menu-bar>');
var itemEl = angular.element(template);

menuBarMock.append(itemEl);
radioMenuItem = $compile(itemEl)($rootScope);

$rootScope.$digest();
});
return radioMenuItem;
Expand Down

0 comments on commit 3780e98

Please sign in to comment.