Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Commit

Permalink
fix(fabActions): corrected use of postLink
Browse files Browse the repository at this point in the history 
properly attach blur/focus listeners on fab-action-items.
  • Loading branch information
@ThomasBurleson
ThomasBurleson committed Jul 7, 2015
1 parent 76cfd70 commit f91a384
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 43 deletions.
32 changes: 17 additions & 15 deletions src/components/fabActions/fabActions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,27 @@
if (children.attr('ng-repeat')) {
children.addClass('md-fab-action-item');
} else {
// After setting up the listeners, wrap every child in a new div and add a class that we can
// scale/fling independently
// Wrap every child in a new div and add a class that we can scale/fling independently
children.wrap('<div class="md-fab-action-item">');
}
},

link: function(scope, element, attributes, controllers) {
// Grab whichever parent controller is used
var controller = controllers[0] || controllers[1];

// Make the children open/close their parent directive
if (controller) {
angular.forEach(element.children(), function(child) {
angular.element(child).on('focus', controller.open);
angular.element(child).on('blur', controller.close);
});

return function postLink(scope, element, attributes, controllers) {
// Grab whichever parent controller is used
var controller = controllers[0] || controllers[1];

// Make the children open/close their parent directive

This comment has been minimized.

Sign in to view

Copy link
@bdteo

bdteo Jul 25, 2015

Why should children open/close parent directive? Isnt only md-fab-trigger supposed to do this?
I believe this is different behavior from what I saw in Google Inbox and child elements opening parent results in worse UX and less flexibility. If someone wants something like this it would be easy enough for him just to use the "md-open" attribute.

This comment has been minimized.

Sign in to view

Copy link
@topherfangio

topherfangio Aug 3, 2015

Contributor

@bdteo Google Inbox doesn't behave like this because in Inbox, you can't actually select the FAB with the keyboard (i.e. you cannot tab to it). The goal of this code was that tabbing through the actions in the speed dial will keep the speed dial open; otherwise, it closes as you go through the actions.

I have already been asked to update the component with better keyboard support so that you tab to the FAB and use the arrow keys to navigate the actions. So, when finished, this code should then be irrelevant and able to be removed.

Sound good?
if (controller) {
angular.forEach(element.children(), function(child) {
// Attach listeners to the `md-fab-action-item`

This comment has been minimized.

Sign in to view

Copy link
@topherfangio

topherfangio Jul 7, 2015

Contributor

I believe this comment is slightly misleading as the listeners should be attached to the child of the md-fab-action-item because the child <button>s are the things that actually gain focus. I believe the code is correct, just the comment is odd.

Am I correct, or missing something?

This comment has been minimized.

Sign in to view

Copy link
@ThomasBurleson

ThomasBurleson Jul 7, 2015

Author Contributor

see next line child = angular.element(child).children()[0];

Issue: will attach listeners to only the first child element of each md-fab-action-item.
child = angular.element(child).children()[0];

angular.element(child).on('focus', controller.open);
angular.element(child).on('blur', controller.close);
});
}
}
}
}
}

})();
})();
76 changes: 48 additions & 28 deletions src/components/fabSpeedDial/fabSpeedDial.spec.js
View file
Original file line number Diff line number Diff line change
@@ -1,21 +1,17 @@
describe('<md-fab-speed-dial> directive', function() {

beforeEach(module('material.components.fabSpeedDial'));
describe('<md-fab-speed-dial> directive', function () {

var pageScope, element, controller;
var $rootScope, $animate, $timeout;

function compileAndLink(template) {
inject(function($compile, $rootScope) {
pageScope = $rootScope.$new();
element = $compile(template)(pageScope);
controller = element.controller('mdFabSpeedDial');

pageScope.$apply();
});
}
beforeEach(module('material.components.fabSpeedDial'));
beforeEach(inject(function (_$rootScope_, _$animate_, _$timeout_) {
$rootScope = _$rootScope_;
$animate = _$animate_;
$timeout = _$timeout_;
}));

it('applies a class for each direction', inject(function() {
compileAndLink(
it('applies a class for each direction', inject(function () {
build(
'<md-fab-speed-dial md-direction="{{direction}}"></md-fab-speed-dial>'
);

Expand All @@ -32,8 +28,8 @@ describe('<md-fab-speed-dial> directive', function() {
expect(element.hasClass('md-right')).toBe(true);
}));

it('opens when the trigger element is focused', inject(function() {
compileAndLink(
it('opens when the trigger element is focused', inject(function () {
build(
'<md-fab-speed-dial><md-fab-trigger><button></button></md-fab-trigger></md-fab-speed-dial>'
);

Expand All @@ -42,32 +38,46 @@ describe('<md-fab-speed-dial> directive', function() {
expect(controller.isOpen).toBe(true);
}));

it('opens when the speed dial elements are focused', inject(function() {
compileAndLink(
it('opens when the speed dial elements are focused', inject(function () {
build(
'<md-fab-speed-dial><md-fab-actions><button></button></md-fab-actions></md-fab-speed-dial>'
);

element.find('button').triggerHandler('focus');
pageScope.$digest();

expect(controller.isOpen).toBe(true);
}));

it('closes when the speed dial elements are blurred', inject(function() {
compileAndLink(
'<md-fab-speed-dial><md-fab-actions><button></button></md-fab-actions></md-fab-speed-dial>'
it('closes when the speed dial elements are blurred', inject(function () {
build(
'<md-fab-speed-dial>'+
' <md-fab-trigger>' +
' <button>Show Actions</button>' +
' </md-fab-trigger>' +
' </md-fab-actions>' +
' <md-fab-actions>' +
' <button>Action 1</button>' +
' </md-fab-actions>' +
'</md-fab-speed-dial>'
);

element.find('button').triggerHandler('focus');
pageScope.$digest();

expect(controller.isOpen).toBe(true);

element.find('button').triggerHandler('blur');
var actionBtn = element.find('md-fab-actions').find('button');
actionBtn.triggerHandler('focus');
pageScope.$digest();
actionBtn.triggerHandler('blur');
pageScope.$digest();

expect(controller.isOpen).toBe(false);
}));

it('allows programmatic opening through the md-open attribute', inject(function() {
compileAndLink(
it('allows programmatic opening through the md-open attribute', inject(function () {
build(
'<md-fab-speed-dial md-open="isOpen"></md-fab-speed-dial>'
);

Expand All @@ -83,8 +93,8 @@ describe('<md-fab-speed-dial> directive', function() {
expect(controller.isOpen).toBe(false);
}));

it('properly finishes the fling animation', inject(function(mdFabSpeedDialFlingAnimation) {
compileAndLink(
it('properly finishes the fling animation', inject(function (mdFabSpeedDialFlingAnimation) {
build(
'<md-fab-speed-dial md-open="isOpen" class="md-fling">' +
' <md-fab-trigger><button></button></md-fab-trigger>' +
' <md-fab-actions><button></button></md-fab-actions>' +
Expand All @@ -101,8 +111,8 @@ describe('<md-fab-speed-dial> directive', function() {
expect(removeDone).toHaveBeenCalled();
}));

it('properly finishes the scale animation', inject(function(mdFabSpeedDialScaleAnimation) {
compileAndLink(
it('properly finishes the scale animation', inject(function (mdFabSpeedDialScaleAnimation) {
build(
'<md-fab-speed-dial md-open="isOpen" class="md-fling">' +
' <md-fab-trigger><button></button></md-fab-trigger>' +
' <md-fab-actions><button></button></md-fab-actions>' +
Expand All @@ -119,4 +129,14 @@ describe('<md-fab-speed-dial> directive', function() {
expect(removeDone).toHaveBeenCalled();
}));

function build(template) {
inject(function ($compile) {
pageScope = $rootScope.$new();
element = $compile(template)(pageScope);
controller = element.controller('mdFabSpeedDial');

pageScope.$apply();
});
}

});

0 comments on commit f91a384

Please sign in to comment.