7
\$\begingroup\$

To illustrate some algorithmic problems I found while writing this code review, I needed to include a live demonstration, which I implemented using a Stack Snippet with Angular.js.

The demonstration takes a color input and a scalar parameter, and produces two read-only color outputs. The "Original" output is known to behave erratically (which is the point of the demo).

However, the HTML feels a bit repetitive. How can I use Angular more effectively? Optional bonus question: would your advice change if I wanted to present one of the outputs as HSV values instead of RGB?

I also think that the CSS is sloppy and not robust to various sizing conditions. Recommendations in that area would also be appreciated.

(For this review, I suggest treating the details of the color manipulation routines as black boxes. For those issues, you may comment on the original answer instead.)

var ColorTweakerCtrl = function ColorTweakerCtrl($scope) {
  $scope.inColor  = new RGBColor(234, 150, 0);
  $scope.diff = 127;
  
  $scope.tweak = function tweak() {
    $scope.inColor.r = Math.round($scope.inColor.r);
    $scope.inColor.g = Math.round($scope.inColor.g);
    $scope.inColor.b = Math.round($scope.inColor.b);
    $scope.diff = Math.round($scope.diff);
    $scope.outColor = tweakColor($scope.inColor, $scope.diff);
    $scope.newColor = newTweakColor($scope.inColor, $scope.diff);
  };
  $scope.tweak();
};


function RGBColor(r, g, b) {
  this.r = r; this.g = g; this.b = b;
}

RGBColor.prototype.toString = function() {
  return 'rgb(' + Math.round(this.r) + ',' + Math.round(this.g) + ',' + Math.round(this.b) + ')';
};

/* Based on formulas from http://www.rapidtables.com/convert/color/rgb-to-hsv.htm */
RGBColor.prototype.toHSV = function toHSV() {
  var r = this.r / 255,
      g = this.g / 255,
      b = this.b / 255;
  var cMax = Math.max(r, g, b),
      cMin = Math.min(r, g, b);
  var Δ = cMax - cMin;
  var hue = 60 * ( (cMax == r) ? ((g - b) / Δ) % 6
                 : (cMax == g) ? ((b - r) / Δ) + 2
                               : ((r - g) / Δ) + 4 );
  var sat = cMax == 0 ? 0 : Δ / cMax;
  var val = cMax;
  return new HSVColor((hue + 360) % 360, sat, val);
}

function HSVColor(h, s, v) {
  this.h = h; this.s = s; this.v = v;
}

HSVColor.prototype.toString = function() {
  return 'hsl(' + Math.round(this.h) + ',' + Math.round(100 * this.s) + '%,' + Math.round(100 * this.v) + '%)';
};

/* http://en.wikipedia.org/wiki/HSL_and_HSV#From_HSV */
HSVColor.prototype.toRGB = function toRGB() {
  var c = this.v * this.s;
  var h = this.h / 60;
  var x = c * (1 - Math.abs(h % 2 - 1));
  var r1 = (h < 1 || h >= 5) ? c
         : (h < 2 || h >= 4) ? x : 0;
  var g1 = (h >= 1 && h < 3) ? c
         : (h >= 0 && h < 4) ? x : 0;
  var b1 = (h >= 3 && h < 5) ? c
         : (h >= 2 && h < 6) ? x : 0;
  var m = this.v - c;
  return new RGBColor(Math.round(255 * (r1 + m)),
                      Math.round(255 * (g1 + m)),
                      Math.round(255 * (b1 + m)));
};


function tweakColor(aColor, aDiff) {
  function intRange(value, min, max) {
    return value < min ? min
         : value > max ? max
                       : value;
  }
  var r = aColor.r,
      g = aColor.g,
      b = aColor.b;
  var d = (r + g + b) / 3;
  var dir = (d >= (256 / 2)) ? -aDiff : aDiff;
  r = intRange(r + dir, 0, 255);
  g = intRange(g + dir, 0, 255);
  b = intRange(b + dir, 0, 255);
  return new RGBColor(r, g, b);
}

function newTweakColor(aColor, aDiff) {
  var hsv = aColor.toHSV();
  var vAdj = (aDiff / 128) * (hsv.v - 0.5);
  return (new HSVColor(hsv.h, hsv.s, hsv.v - vAdj)).toRGB();
}
fieldset {
  display: inline-block;
  width: 25%;
}
fieldset#tweak {
  display: block;
  width: 100%;
  border-width: 0;
}
label {
  display: block;
}
.swatch {
  border: 1px solid grey;
  width: 100%;
  height: 1em;
}
input[type=range] {
  width: 40%;
}
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.23/angular.min.js"></script>
<div ng-app>
  <form ng-controller="ColorTweakerCtrl">
    <fieldset id="tweak">
      <label>Diff
        <input ng-model="diff" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="diff" type="number" min="0" max="255" ng-change="tweak()">
      </label>
    </fieldset>
    
    <fieldset>
      <legend>Input Color</legend>
      <div class="swatch" style="background-color: {{ inColor.toString() }}"></div>
      <label>R
        <input ng-model="inColor.r" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="inColor.r" type="number" min="0" max="255" ng-change="tweak()">
      </label>
      <label>G
        <input ng-model="inColor.g" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="inColor.g" type="number" min="0" max="255" ng-change="tweak()">
      </label>
      <label>B
        <input ng-model="inColor.b" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="inColor.b" type="number" min="0" max="255" ng-change="tweak()">
      </label>
    </fieldset>

    <fieldset>
      <legend>Output (Original)</legend>
      <div class="swatch" style="background-color: {{ outColor.toString() }}"></div>
      <label>R
        <input ng-model="outColor.r" type="range" min="0" max="255" disabled>
        <input ng-model="outColor.r" type="number" min="0" max="255" disabled>
      </label>
      <label>G
        <input ng-model="outColor.g" type="range" min="0" max="255" disabled>
        <input ng-model="outColor.g" type="number" min="0" max="255" disabled>
      </label>
      <label>B
        <input ng-model="outColor.b" type="range" min="0" max="255" disabled>
        <input ng-model="outColor.b" type="number" min="0" max="255" disabled>
      </label>
    </fieldset>


    <fieldset>
      <legend>Output (Suggested)</legend>
      <div class="swatch" style="background-color: {{ newColor.toString() }}"></div>
      <label>R
        <input ng-model="newColor.r" type="range" min="0" max="255" disabled>
        <input ng-model="newColor.r" type="number" min="0" max="255" disabled>
      </label>
      <label>G
        <input ng-model="newColor.g" type="range" min="0" max="255" disabled>
        <input ng-model="newColor.g" type="number" min="0" max="255" disabled>
      </label>
      <label>B
        <input ng-model="newColor.b" type="range" min="0" max="255" disabled>
        <input ng-model="newColor.b" type="number" min="0" max="255" disabled>
      </label>
    </fieldset>
  </form>
</div>

\$\endgroup\$
3
\$\begingroup\$

Markup

Your markup is invalid. A label is only allowed to be associated with a single form element.

A color input element would be a more semantically appropriate choice over that div.

CSS

For the RGB label text, I recommend switching to a mono-spaced font. As it stands, your form elements will not line up because each letter varies in width.

I don't see much reason for your number fields to be any wider than what's necessary for the the widest number value visible (which might not be the highest number!). A width of 3em-4em would probably be a good starting point.

\$\endgroup\$
  • \$\begingroup\$ Unfortunately, support for <input type="color"> is currently limited to Firefox, Chrome, and Opera. Also, there is no telling what that widget would look like or how it would behave, as the spec is fuzzy. \$\endgroup\$ – 200_success Dec 3 '14 at 4:41
  • \$\begingroup\$ There's no guarantees as to how any form element will look (notably, checkbox and file vary depending on browser/os). An element should be chosen should be based on its purpose, not because of how it looks. As a user, I only care how a particular element looks on my browser, how it looks on other browsers (whether it be the same or different) is of no concern to me. \$\endgroup\$ – cimmanon Dec 3 '14 at 12:06
  • \$\begingroup\$ A palette of 8 fixed color choices would be within the spec for <input type="color">. Furthermore, the fallback on browsers that don't support it yet is a text field. \$\endgroup\$ – 200_success Dec 3 '14 at 16:01
  • \$\begingroup\$ what do you mean by A label is only allowed to be associated with a single form element I only see a single form element. \$\endgroup\$ – Malachi Dec 3 '14 at 16:46
  • \$\begingroup\$ @Malachi I guess I should have said form controls, not form element. Error from the validator: "label element with multiple labelable descendants" and "The label element may contain at most one input, button, select, textarea, or keygen descendant." \$\endgroup\$ – cimmanon Dec 3 '14 at 16:54
1
\$\begingroup\$

You should change your code from Labels to Tables I think

That way the Syntax is happy and everything still operates the same way.

something like this

var ColorTweakerCtrl = function ColorTweakerCtrl($scope) {
  $scope.inColor  = new RGBColor(234, 150, 0);
  $scope.diff = 127;
  
  $scope.tweak = function tweak() {
    $scope.inColor.r = Math.round($scope.inColor.r);
    $scope.inColor.g = Math.round($scope.inColor.g);
    $scope.inColor.b = Math.round($scope.inColor.b);
    $scope.diff = Math.round($scope.diff);
    $scope.outColor = tweakColor($scope.inColor, $scope.diff);
    $scope.newColor = newTweakColor($scope.inColor, $scope.diff);
  };
  $scope.tweak();
};


function RGBColor(r, g, b) {
  this.r = r; this.g = g; this.b = b;
}

RGBColor.prototype.toString = function() {
  return 'rgb(' + Math.round(this.r) + ',' + Math.round(this.g) + ',' + Math.round(this.b) + ')';
};

/* Based on formulas from http://www.rapidtables.com/convert/color/rgb-to-hsv.htm */
RGBColor.prototype.toHSV = function toHSV() {
  var r = this.r / 255,
      g = this.g / 255,
      b = this.b / 255;
  var cMax = Math.max(r, g, b),
      cMin = Math.min(r, g, b);
  var Δ = cMax - cMin;
  var hue = 60 * ( (cMax == r) ? ((g - b) / Δ) % 6
                 : (cMax == g) ? ((b - r) / Δ) + 2
                               : ((r - g) / Δ) + 4 );
  var sat = cMax == 0 ? 0 : Δ / cMax;
  var val = cMax;
  return new HSVColor((hue + 360) % 360, sat, val);
}

function HSVColor(h, s, v) {
  this.h = h; this.s = s; this.v = v;
}

HSVColor.prototype.toString = function() {
  return 'hsl(' + Math.round(this.h) + ',' + Math.round(100 * this.s) + '%,' + Math.round(100 * this.v) + '%)';
};

/* http://en.wikipedia.org/wiki/HSL_and_HSV#From_HSV */
HSVColor.prototype.toRGB = function toRGB() {
  var c = this.v * this.s;
  var h = this.h / 60;
  var x = c * (1 - Math.abs(h % 2 - 1));
  var r1 = (h < 1 || h >= 5) ? c
         : (h < 2 || h >= 4) ? x : 0;
  var g1 = (h >= 1 && h < 3) ? c
         : (h >= 0 && h < 4) ? x : 0;
  var b1 = (h >= 3 && h < 5) ? c
         : (h >= 2 && h < 6) ? x : 0;
  var m = this.v - c;
  return new RGBColor(Math.round(255 * (r1 + m)),
                      Math.round(255 * (g1 + m)),
                      Math.round(255 * (b1 + m)));
};


function tweakColor(aColor, aDiff) {
  function intRange(value, min, max) {
    return value < min ? min
         : value > max ? max
                       : value;
  }
  var r = aColor.r,
      g = aColor.g,
      b = aColor.b;
  var d = (r + g + b) / 3;
  var dir = (d >= (256 / 2)) ? -aDiff : aDiff;
  r = intRange(r + dir, 0, 255);
  g = intRange(g + dir, 0, 255);
  b = intRange(b + dir, 0, 255);
  return new RGBColor(r, g, b);
}

function newTweakColor(aColor, aDiff) {
  var hsv = aColor.toHSV();
  var vAdj = (aDiff / 128) * (hsv.v - 0.5);
  return (new HSVColor(hsv.h, hsv.s, hsv.v - vAdj)).toRGB();
}
fieldset {
  display: inline-block;
  width: 25%;
}
fieldset#tweak {
  display: block;
  width: 100%;
  border-width: 0;
}
label {
  display: block;
}
.swatch {
  border: 1px solid grey;
  width: 100%;
  height: 1em;
}
input[type=range] {
  width: 40%;
}
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.23/angular.min.js"></script>
<div ng-app>
  <form ng-controller="ColorTweakerCtrl">
    <fieldset id="tweak">
      <label>Diff
        <input ng-model="diff" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="diff" type="number" min="0" max="255" ng-change="tweak()">
      </label>
    </fieldset>
    
    <fieldset>
        <legend>Input Color</legend>
        <table>
            <tr>
                <td colspan="3">
                    <div class="swatch" style="background-color: {{ inColor.toString() }}"></div>
                <td>
            </tr>
            <tr>
                <td> R </td>
                <td> 
                    <input ng-model="inColor.r" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="inColor.r" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
            <tr>
                <td> G </td>
                <td> 
                    <input ng-model="inColor.g" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="inColor.g" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
            <tr>
                <td> B </td>
                <td> 
                    <input ng-model="inColor.b" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="inColor.b" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
        </table>
    </fieldset>

    <fieldset>
        <legend>Output (Original)</legend>
        <table>
            <tr>
                <td colspan="3">
                    <div class="swatch" style="background-color: {{ outColor.toString() }}"></div>
                <td>
            </tr>
            <tr>
                <td> R </td>
                <td> 
                    <input ng-model="outColor.r" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="outColor.r" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
            <tr>
                <td> G </td>
                <td> 
                    <input ng-model="outColor.g" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="outColor.g" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
            <tr>
                <td> B </td>
                <td> 
                    <input ng-model="outColor.b" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="outColor.b" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
        </table>
    </fieldset>

    <fieldset>
        <legend>Output (Suggested)</legend>
        <table>
            <tr>
                <td colspan="3">
                    <div class="swatch" style="background-color: {{ newColor.toString() }}"></div>
                <td>
            </tr>
            <tr>
                <td> R </td>
                <td> 
                    <input ng-model="newColor.r" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="newColor.r" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
            <tr>
                <td> G </td>
                <td> 
                    <input ng-model="newColor.g" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="newColor.g" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
            <tr>
                <td> B </td>
                <td> 
                    <input ng-model="newColor.b" type="range" min="0" max="255" ng-change="tweak()" />
                </td>
                <td>
                    <input ng-model="newColor.b" type="number" min="0" max="255" ng-change="tweak()" />
                </td>
            <tr>
        </table>
    </fieldset>
  </form>
</div>

\$\endgroup\$
  • \$\begingroup\$ The width: 40% rule now means 40% of the width of the cell rather than 40% of the width of the fieldset, which is why the slider is now too short. \$\endgroup\$ – 200_success Dec 3 '14 at 21:02
  • \$\begingroup\$ that makes sense...lol. if I remove that they get too wide, I will leave as is but take out the comment, it still functions but needs to be tweaked a little bit. \$\endgroup\$ – Malachi Dec 3 '14 at 21:06

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.