Angular dropdown

I have created this little dropdown directive in Angular. The idea is that you create a nav element with a ul inside. The directive prepends a chevron and toggles scope.visible when you click it. Two functions watch scope.visible, the first folds the dropdown, and the second reverses the chevron.

I'm attacking this from a jQuery perspective so I suspect I'm not doing this in the optimal way. How can I improve this code to make it more testable, more readable, and more Angular?

Is it good to create all these little helper methods, or should I be splitting my code out into smaller pieces?

myApp.directive('dropdown', function () {
  return {
    scope: true,
    link: function (scope, element) {
      var menu = element.find('ul');
      var body = angular.element('body');
      var unfold = angular.element("");
      var toggleVisible = function () {
        scope.menu.visible = !scope.menu.visible;
        scope.$apply();
      };
      var unsetVisible = function () {
        scope.menu.visible = false;
        scope.$apply();
      };
      var showMenu = function () {
        if (scope.menu.visible) {
          menu.show();
        } else {
          menu.hide();
        }
      };
      var toggleChevron = function () {
        var c;
        if (scope.menu.visible) {
          c = 'fa-caret-up';
        } else {
          c = 'fa-caret-down';
        }
        unfold.find('i').attr('class', c);
      };
      var escapeKey = function (e) {
        if (e.which === 27) {
          unsetVisible();
        }
      };
      var chevronClick = function (e) {
        toggleVisible();
        e.stopPropagation();
      };

      element.prepend(unfold);
      scope.menu = {visible: false};
      unfold.on('click', chevronClick);
      body.on('click', unsetVisible);
      body.on('keyup', escapeKey);
      scope.$watch('menu.visible', showMenu);
      scope.$watch('menu.visible', toggleChevron);
    }
  };
});

The Rule of Simplicity is getting neglected a bit, but broadly, this code
isn't bad.

You're doing a fairly simple thing here, so the code would me clearer with less
indirection. You don't need to abstract away toggleChevron and chevronClick
separately as you only use each of those actions once and they act unsurprisingly.

myApp.directive('dropdown', function () {
return {
scope: true,
link: function (scope, elem) {


The body of this function is broken into sections of related actions in the
order they occur. This makes it easier to read.

// dom setup
var body = angular.element('body')
var menu = elem.find('ul')
var unfold = angular.element("")
var icon = unfold.find('i')

elem.prepend(unfold)


In my experience, multiple watchers can make for some strange stack traces when
debugging. With modifications this simple, it makes sense to only watch once.

// observe scope
scope.menu = {visible: false}
scope.$watch('menu.visible', update)


Map user triggered events to the actions they should trigger. The only place
the reader needs to understand escapeKey is as it relates to
body.on('keyup'). Dealing with events in anonymous functions and then calling
the scope modifiers reduces the hunting the reader has to do to discover what
actions triggered by events. With many more events, you might do this differently.

// user interactions
body.on('click', unsetVisible)
body.on('keyup', function (e) {
// escape key
if (e.which === 27) unsetVisible(e)
})
unfold.on('click', function (e) {
e.stopPropagation()
toggleVisible()
})


We're only doing two different things, so we only have two modification methods.

// scope modifications
function unsetVisible () {
scope.menu.visible = false
scope.$apply()
}

function toggleVisible () {
scope.menu.visible = !scope.menu.visible
scope.$apply()
}


Two one-line dom changes can live in a single function happily. If you were doing a
ton of dom manipulation here, you would break this up.

$watch passes the new value as the first argument to the watch function,
so you can save a bit of typing in this function.

// update ui
function update (visible) {
// flip chevron
icon.attr('class', visible ? 'fa-caret-up' : 'fa-caret-down')
// toggle menu
menu.toggle(visible)
}
}
}
})


It was a stylistic choice to leave out semicolons for legibility. A good
overview about automatic semicolon insertion lives here.

A note on making this more resilient: here's how to use this directive in html:





You might consider having this directive add the `` element if it isn't
present.

StackExchange Code Review Q#50944, answer score: 3