Angular dropdown

Question

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("<a href='#'><i/></a>");
      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);
    }
  };
});

Answer 1

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("<a href='#'><i/></a>")
      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:

<!-- <ul> child is required -->
<div dropdown><ul></ul></div>

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