0
\$\begingroup\$

I have created a directive for common form fields shared across all the forms in a large AngularJS app I am re-factoring.

The app has lots of forms for various data models (user, owners, victims, suppliers, etc). A lot of the forms for these data models contain an address section which is the same format throughout the app.

  • Address Line 1
  • Address Line 2
  • Town / City
  • County
  • Postcode
  • Country

Here is the markup for the directive:

 <address-dir
     address-line-one="user.address_line_1"
     address-line-two="user.address_line_2"
     address-town-city="user.address_town_city"
     address-county="user.county"
     address-postcode="user.postcode"
     address-country="user.address_country"
 >
 </address-dir>

The actual angular directive code is:

(function () {
    var app = angular.module('userDirectives', []);

    app.directive('addressDir', function () {
        return {
            restrict: "AE",
            require: '^form',
            scope: {
                addressLineOne: '=',
                addressLineTwo: '=',
                addressTownCity: '=',
                addressCounty: '=',
                addressPostcode: '=',
                addressCountry: '='
            },
            templateUrl: 'address.html',
            link: function (scope, element, attributes, form) {
                scope.addressForm = form;
            }
        };
    });
}());

Here is my address.html template which is the template used in the directive (I have removed all but the first field to simplify the example):

<div>
    <label for="address_1">
        Address Line 1
    </label>
    <input
        type="text"
        id="address_1"
        name="address_1"
        ng-model="addressLineOne"
        required
        placeholder="address line 1"
    >
    <div ng-if="(addressForm.$submitted || addressForm.address_1.$touched)" ng-messages="addressForm.address_1.$error" class="error" role="alert">
        <div class="error" ng-message="required">You did not enter first line of address</div>
    </div>
</div>

Here's a simple controller:

(function () {
    var app = angular.module('userControllers', []);

    app.controller('UserController', ['$scope', function ($scope) {

        $scope.user = {};

        // This is called when the form is sumitted

        $scope.continue = function () {

            console.log($scope.user);

            // save here

        };

        // get user
        User.get().then(function (user) {
            $scope.user = user;
        });

    }]);

}());

I'm working on the project solo for a while and I couldn't find any other directives that do something like this on the net (or at least for common fields in forms).

The reason I have made this a directive instead of a basic include is because I next want to add some custom DOM manipulations to the address section on some of the forms. Probably some third-party address lookup / validation integration.

It is quite basic at moment but I am looking for some external feedback on what I have so far and the direction I'm going in.

\$\endgroup\$
5
  • \$\begingroup\$ Your "Get User" thing is indicative of a code smell - controllers should only retrieve data and assign it to the view. This isn't lengthy enough for a review so leaving as a comment. \$\endgroup\$
    – Dan
    Sep 9 '15 at 16:03
  • \$\begingroup\$ that is all it is currently doing? \$\endgroup\$ Sep 9 '15 at 17:58
  • \$\begingroup\$ When I say retrieve data, I mean from the Angular injector. Calling a service to retrieve data asynchronously isn't a great idea in a controller because your controller apparently relies on that field (the user field) to fulfill its job properly. However, by the end of the constructor, you don't have all of the data required for the controller to do it's job and thus, the constructor's porpose is voided \$\endgroup\$
    – Dan
    Sep 9 '15 at 17:59
  • \$\begingroup\$ Ah right yes I see. I actually already have the user data in the service before this controller can be reached anyhow so it doesn't need to be called asynchronously like I did above. \$\endgroup\$ Sep 9 '15 at 18:11
  • \$\begingroup\$ You may want to use optional bidirectional binding for addressLineTwo as in addressLineTwo: '=?' as this is often not required and a consumer of your directive should not have to bind to it. \$\endgroup\$ Sep 22 '16 at 6:19
1
\$\begingroup\$
address-dir

What is "dir"? Is there something special about "dir"? It would be best to provide a very common and understandable name to directives. That way, new devs wouldn't be scratching their heads and waste the next 20 mins figuring out what it does. I would name it <address-field> or <address-input>.

address-*

The prefix is redundant. We already know it's an address field.

address-town-city

Sounds a bit redundant. I'd go with either one, but "city" would be more common in some cases.

address-postcode

Would "zip-code" or just "zip" be a better name? Not sure what it's called in other places, but that's what I normally see in shopping sites.

scope.addressForm = form;

I would prefer that you not let the directive know about the form or anything from the outside world for that matter. This limits the directive's portability. Additionally, it's an implicit dependency which nobody will find out until one actually looks into the implementation - which defeats the purpose of an interface.

It's best that a directive should not make assumptions of the outside world and only know about the things inside it. For it to know something from the outside world, it's best that you explicitly define an interface, like a scope variable. In this case, you want to know if the form was submitted. Why not design a scope variable has-submitted.

Now for suggestions:

  • You might also want to add in your own required property to require all fields, or several require-* to require separate fields. That's because address line 2 and county are usually optional.

  • You might also want to add in show-* properties, so that you can conditionally hide fields.

  • Built-in values for countries and cities. That way, it's not a free text input, which should limit chances of error. There's a lot of libraries you could use for this one.

  • Built-in validation. The usual checks like whitespace/blank checks, value and length checks. I think ng-pattern covers most cases, but you could pull in a validation library if you want.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the tips on renaming (always good to not over complicate) and also about not having form as an implicit dependency. \$\endgroup\$ Sep 9 '15 at 15:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.