current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

This project has evolved a lot since I started learning Angular. I'm pretty happy with it now, and I'm wondering if there's anything I can improve upon, or have forgotten at this point...

HTML

<h1 data-fittext>FitText</h1>

<h1 data-fittext=".315" data-fittext-min="12" data-fittext-max="50">ng-FitText</h1>

Inclusion & Customization

var myApp = angular.module( 'myApp', [ 'ngFitText' ] );

myApp.config( function( fitTextConfigProvider ) {
    fitTextConfigProvider.config = {
        debounce: true, //default is false
        delay: 1000 //default is 250
    };

    // OR

    fitTextConfigProvider.config.debounce = true;
    fitTextConfigProvider.config.delay = 1000;
});

The Module

'use strict';

angular.module( 'ngFitText', [] )
    .value( 'config', {
        'debounce': false,
        'delay': 250
    })

    .directive( 'fittext', [ 'config', 'fitTextConfig', function( config, fitTextConfig ) {
        return {
            restrict: 'A',
            scope: true,
            transclude: true,
            replace: true,
            template: function( element, attrs ) {
                var tag = element[0].nodeName;
                return "<"+tag+" data-ng-transclude data-ng-style='{fontSize:fontSize}'></"+tag+">";
            },
            link: function( scope, element, attrs ) {
                angular.extend(config, fitTextConfig.config);

                scope.compressor = attrs.fittext || 1;
                scope.minFontSize = attrs.fittextMin || Number.NEGATIVE_INFINITY;
                scope.maxFontSize = attrs.fittextMax || Number.POSITIVE_INFINITY;
                scope.elementWidth = element[0].offsetWidth;

                ( scope.resizer = function() {
                    scope.elementWidth = element[0].offsetWidth;
                    scope.fontSize = Math.max(
                        Math.min(
                            scope.elementWidth / ( scope.compressor * 10 ),
                            parseFloat( scope.maxFontSize )
                        ),
                        parseFloat( scope.minFontSize )
                    ) + 'px';

                    if( !scope.$$phase ) scope.$digest();
                })();

                config.debounce == true
                    ? angular.element( window ).bind( 'resize', debounce( scope.resizer, config.delay ))
                    : angular.element( window ).bind( 'resize', scope.resizer);

                function debounce(a,b,c){var d;return function(){var e=this,f=arguments;clearTimeout(d),d=setTimeout(function(){d=null,c||a.apply(e,f)},b),c&&!d&&a.apply(e,f)}}
            }
        }
    }])

    .provider( 'fitTextConfig', function() {
        var self = this;
        this.config = {};
        this.$get = function() {
            var extend = {};
            extend.config = self.config;
            return extend;
        };
        return this;
    });
share|improve this question

1 Answer 1

up vote 2 down vote

  • As personal opinion, I wouldn't use config as name for .value, it sounds too generic and can be confused with .config method of the module API.

  • Putting comments like //default is false looks confusing - why are these not default? Also it is easy to change the defaults and forget to adjust comments. Uncle Bob recommends to minimize comments and use expressive code with descriptive names instead.

  • function debounce(a,b,c) is great for minimizers but cryptic for code readers

  • In my view, using .provider is possibly an overkill as there is not much configuration logic here.

  • The last two comments here also apply. Instead the config JSON can be kept inside .value and decoupled from the provider.

share|improve this answer
    
Thanks for your points. The debounce function is taken from Underscore and stated as such in the module comments. Overkill doesn't concern me, but is there another way to extend configuration? As far as I'm concerned it's not a configurable module if config settings are blown away upon updating the module... –  Patrick Apr 22 '14 at 17:00
    
Could you explain how $$phase, in this context, is an anti-pattern? It seems pretty legitimate to me as $apply is already called on load, but is required when the window resizes to apply the updated font size. I've modified the code to remove this anti-pattern, but you essentially end up with something fairly messy in comparison: github.com/patrickmarabeas/ng-FitText.js/blob/master/… I can understand in a lot of cases its usage means you are probably doing something wrong - but it seems like a more succinct readable solution here? –  Patrick Apr 29 '14 at 7:56
    
I understand that $$phase is Angular's internal method, and as such is not intended for regular use. Please see here for more specific details: stackoverflow.com/questions/22346990/…. –  Dmitri Zaitsev Apr 29 '14 at 14:04
    
Concerning debounce, you could take it instead from the non-minimized version with comments, then another reader would better see what it does. –  Dmitri Zaitsev Apr 29 '14 at 14:06
    
See also bennadel.com/blog/… about $$phase –  Dmitri Zaitsev Dec 29 '14 at 8:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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