Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

tools.parseUnitString and tools.parseAngle #167

Merged
merged 8 commits into from

3 participants

@iamdustan

Building upon these previous conversations, here are some converting methods to start accepting angles in any format throughout the project.

#142 (comment)
#165 (comment)

+@basecode

src/tools.js
@@ -339,6 +340,45 @@ define([], function() {
array.length -= numRemoved;
return array;
+ },
+
+ /**
+ * Parse a given unit string into the amount and unit
+ *
+ * @param {String} str The unit string. E.g. '90deg', '7em'
+ * @returns {Object} { amount, unit }
+ */
+ parseUnitString: function (str) {
+ var amount = parseFloat(str.match(/^\-?\d+/)),
@basecode Owner
basecode added a note

I think parseFloat works quite well without the regex.

haha. very good point. It was early when I did that, I swear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/tools.js
((12 lines not shown))
+ parseUnitString: function (str) {
+ var amount = parseFloat(str.match(/^\-?\d+/)),
+ unit = str.match(/[a-z]*$/i)[0];
+
+ return { amount: amount, unit: unit };
+ },
+
+ /**
+ * Parses the angle string to radians
+ * https://developer.mozilla.org/en-US/docs/CSS/angle
+ *
+ * @param {String} angle The angle/unit string
+ * @returns {Number} The angle in radians
+ */
+ parseAngle: function(angle) {
+ var parts = tools.parseUnitString(angle), unit = parts.amount, radians;
@basecode Owner
basecode added a note

Shouldn't it be amount = parts.amount instead of unit?

Yes, yes it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@basecode
Owner

I like the idea of supporting units. tools.parseAngle sounds good. Not sure if we need tools.parseUnitString though.

@iamdustan

parseUnitString was meant to be more low level. Currently it’s motivated to be the string-to-parts work of parseAngle and letting parseAngle really just deal with the conversion, but other unit types have different units of measure. E.g. 1s 1000ms 3min 50%

@basecode
Owner

That makes sense.

src/tools.js
((8 lines not shown))
+ *
+ * @param {String} str The unit string. E.g. '90deg', '7em'
+ * @returns {Object} { amount, unit }
+ */
+ parseUnitString: function (str) {
+ var amount = parseFloat(str),
+ unit = str.match(/[a-z]*$/i)[0];
+
+ return { amount: amount, unit: unit };
+ },
+
+ /**
+ * Parses the angle string to radians
+ * https://developer.mozilla.org/en-US/docs/CSS/angle
+ *
+ * @param {String} angle The angle/unit string
@basecode Owner
basecode added a note

parseAngle should also allow numbers (that are handled as radians). That would allow us to parse the parameter independent from the type. var radians = tools.parseAngle(param);

Good catch.

Simple enough as if (typeof angle === 'number') return angle; or should there be more robust handling of sorts (not sure what that would be...)

@basecode Owner
basecode added a note

I'm tempted to let parseUnitString only return the unit and not "amount". For three reasons:
1) parseFloat is nothing we need to put in a helper function. It is already a well tested helper function itself and can safely be used.
2) We don't need to allocate another object (to be able to return 2 values)
3) It saves use from using if-else

Code:

extractUnit: function (any) {
  // make sure it's a string and remove trailing whitespace
  var unit = String(any).replace(/\s+$/, '');
  // returns extracted unit or empty string
  return unit.match(/[a-z]*$/i)[0];
}
parseAngle: function(angle) {

  var radians;
  var amount = parseFloat(angle);
  var unit = extractUnit(angle);

  switch(unit) {
    case 'deg': 
      radians = amount * PI / 180;
      break;
    …
    case '': // default
    case 'rad':
      radians = amount;
      break;
  }

  return radians;

}

What do you think?

:thumbsup:

I don’t know why I had completely blanked and not thought of 1. I also disliked it very much for number two so that is a big one. Should we handle someone having white space between the number and unit as well?

'7 deg'?

@basecode Owner
basecode added a note

extractUnit("7 deg") // "deg"
That's fine, isn't it? Your regex does already the work of ignoring leading whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@iamdustan

:thumbsup: Any other cases we can think of to test and handle?

src/tools.js
((12 lines not shown))
+ parseUnitString: function (any) {
+ // make sure it's a string and remove trailing whitespace
+ var unit = String(any).replace(/\s+$/, '');
+ // returns extracted unit or empty string
+ return unit.match(/[a-z]*$/i)[0];
+ },
+
+ /**
+ * Parses the angle string to radians
+ * https://developer.mozilla.org/en-US/docs/CSS/angle
+ *
+ * @param {Number|String} angle The angle/unit string
+ * @returns {Number} The angle in radians
+ */
+ parseAngle: function(angle) {
+ var unit = tools.parseUnitString(angle),
@basecode Owner
basecode added a note

Please put radians at the beginning. Variables that are not explicitly assigned should be listed first. I know there aren't any coding-styles available yet for bonsai.. but soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@iamdustan

......annnnnnnnd done. I hadn’t even realized that nuance in the bonsai style. I’ve been trying to keep my additions like they belong in the family :)

src/tools.js
@@ -339,6 +340,48 @@ define([], function() {
array.length -= numRemoved;
return array;
+ },
+
+ /**
+ * Parse a given unit string into the amount and unit
+ *
+ * @param {String} any The unit string. E.g. '90deg', '7em'
+ * @returns {String} unit
+ */
+ parseUnitString: function (any) {
@basecode Owner
basecode added a note

Since parseUnitString can not just parse strings but also arrays (parseUnitString(["7em"])) for example wouldn't it be better to rename parseUnitString to extractUnit and change the JSDOC's "@ param" to {any}?

Yup. That does make sense. I didn’t even think of the example of passing an array in. Update coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/tools.js
((26 lines not shown))
+ parseAngle: function(angle) {
+ var radians,
+ unit = tools.parseUnitString(angle),
+ amount = parseFloat(angle);
+
+ switch (unit) {
+ case '': // default
+ case 'rad':
+ radians = amount; break;
+ case 'deg':
+ radians = amount * PI / 180; break;
+ case 'grad':
+ radians = amount * PI / 200; break;
+ case 'turn':
+ radians = amount * 2 * PI; break;
+ default:
@basecode Owner
basecode added a note

Why do we need "default:" here?

Don’t all switch statements need a default? I don’t often use switch statements. I usually do the dirty super ternary instead :|

var radians = 'rad' === unit ? amount
            : 'deg' === unit ? amount * PI / 180
            : 'grad' === unit ? amount * PI / 200
            : 'turn' === unit ? amount * 2 * PI
            : 0;
@basecode Owner
basecode added a note

Well, I think a switch statement don't need necessarily a "default", especially when the default value was previously set (in L470). You already implicitly assigned undefined.

What do you think of reversing it? Rather than case '' falling through to case 'rad' we let 'rad' fail through to default.

@basecode Owner
basecode added a note

In that case we wouldn't even need case 'rad'. But parseAngle('2whatever') would return 2 (rad). :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/tools-spec.js
((14 lines not shown))
+ });
+
+ it('should work with negative numbers', function() {
+ var returned = parseUnitString('-741232grad');
+ var expected = 'grad';
+
+ expect(returned).toEqual(expected);
+ });
+
+ it('should work with trailing whitespace', function() {
+ var returned = parseUnitString('-741232grad ');
+ var expected = 'grad';
+
+ expect(returned).toEqual(expected);
+ });
+
@basecode Owner
basecode added a note

Other edge cases:

  • should work with leading whitespace
  • should return an empty string in case of a number

Tests for both of those as well as the unit being % and for when undefined is passed in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@basecode basecode commented on the diff
test/tools-spec.js
((40 lines not shown))
+ });
+
+ it('calculates radians for turns', function() {
+ var calculated = parseAngle('1turn');
+ var expected = 6.283185307179586;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+
+ it('calculates radians for gradians', function () {
+ var calculated = parseAngle('-32grad');
+ var expected = -0.5026548245743669;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+
@basecode Owner
basecode added a note

Other edge cases:

  • calculates radians for radians as a string
  • calculates radians for radians as a number

for radians as a string as in parseAngle('0.321512324') or as in parseAngle('0.321512341rad')?

Added tests for the following scenarios:

parseAngle('0.42131') parseAngle(0.42131) and parseAngle('0.42131rad')

@basecode Owner
basecode added a note

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/tools.js
@@ -339,6 +340,52 @@ define([], function() {
array.length -= numRemoved;
return array;
+ },
+
+ /**
+ * Parse a given unit string into the amount and unit
+ *
+ * @param {any} any Anything that can be converted to a string. E.g. '7deg', ['50%']
+ * @returns {String} unit
+ */
+ extractUnit: function (any) {
+ if (typeof any === 'undefined' || typeof any === 'number') {
@basecode Owner
basecode added a note

I don't think that we need that check. What about "", null, [], {}, …? I see your point but I don't think that we should expect undefined and number of all types. The cast in L357 takes care of invalid values.

so do some test after var unit = ... to make sure we return the empty string if given a poor argument?

String(undefined) //=> "undefined"
String(null) //=> "null"
String({}) //=> "[object Object]"
String([]) //=> ''
String(['singleDepth']) //=> 'singleDepth'
String(['bigger', 'array', 4]) //=> 'bigger,array,4'
@basecode Owner
basecode added a note

No. That's sth the author/dev need to fix. That's why I suggested to remove the type-checks.

:thumbsup: I can roll with that. Removed the checks, tests are still passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@basecode
Owner

Oh and please add a line to the changelog :)

@basecode
Owner

And thanks for your endurance! This is going to be a really nice feature!

@iamdustan

The endurance is easy over here! I built it up working on matrices, getters/setters, and compounding transformations that I only half understand.

@iamdustan

Pushed.

@basecode basecode merged commit 46b612d into from
@basecode
Owner

Thanks for this addition @iamdustan ! :+1:

@iamdustan

:) We made it!

@klipstein
Owner

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 137 additions and 2 deletions.
  1. +1 −1  CHANGELOG
  2. +42 −1 src/tools.js
  3. +94 −0 test/tools-spec.js
View
2  CHANGELOG
@@ -1,6 +1,6 @@
v0.4.2
-------------------
-
+* Add tools.parseAngle to accept angle units as strings. http://www.w3.org/TR/css3-values/#angles
* Add support for an `interactive` attribute on all DisplayObjects which allows
pointer-events to be received (true) or to pass through (false)
* Setting an attribute to the actual value won't trigger a render message.
View
43 src/tools.js
@@ -6,7 +6,8 @@ define([], function() {
noop = function() {},
push = [].push,
slice = [].slice,
- toString = {}.toString;
+ toString = {}.toString,
+ PI = Math.PI;
/**
*
@@ -339,6 +340,46 @@ define([], function() {
array.length -= numRemoved;
return array;
+ },
+
+ /**
+ * Parse a given unit string into the amount and unit
+ *
+ * @param {any} any Anything that can be converted to a string. E.g. '7deg', ['50%']
+ * @returns {String} unit
+ */
+ extractUnit: function (any) {
+ // make sure it's a string and remove trailing whitespace
+ var unit = String(any).replace(/\s+$/, '');
+ // returns extracted unit or empty string
+ return unit.match(/[a-z%]*$/i)[0];
+ },
+
+ /**
+ * Parses the angle string to radians
+ * https://developer.mozilla.org/en-US/docs/CSS/angle
+ *
+ * @param {Number|String} angle The angle/unit string
+ * @returns {Number} The angle in radians
+ */
+ parseAngle: function(angle) {
+ var radians,
+ unit = tools.extractUnit(angle),
+ amount = parseFloat(angle);
+
+ switch (unit) {
+ case '': // default
+ case 'rad':
+ radians = amount; break;
+ case 'deg':
+ radians = amount * PI / 180; break;
+ case 'grad':
+ radians = amount * PI / 200; break;
+ case 'turn':
+ radians = amount * 2 * PI; break;
+ }
+
+ return radians;
}
};
View
94 test/tools-spec.js
@@ -284,4 +284,98 @@ define([
});
});
+ /*--------------------------------------------------------------------------*/
+
+ describe('tools.extractUnit', function() {
+ var extractUnit = tools.extractUnit;
+
+ it('return the unit from a unit string', function() {
+ var returned = extractUnit('9deg');
+ var expected = 'deg';
+
+ expect(returned).toBe(expected);
+ });
+
+ it('should work with negative numbers', function() {
+ var returned = extractUnit('-741232grad');
+ var expected = 'grad';
+
+ expect(returned).toBe(expected);
+ });
+
+ it('should work with trailing whitespace', function() {
+ var returned = extractUnit('-741232grad ');
+ var expected = 'grad';
+
+ expect(returned).toBe(expected);
+ });
+
+ it('should work with leading whitespace', function() {
+ var returned = extractUnit('-741232 grad ');
+ var expected = 'grad';
+
+ expect(returned).toBe(expected);
+ });
+
+ it('should return an empty string in case of a number', function () {
+ var returned = extractUnit(7531);
+ var expected = '';
+
+ expect(returned).toBe(expected);
+ });
+
+ it('should return the string when given an array', function () {
+ var returned = extractUnit(['50s']);
+ var expected = 's';
+
+ expect(returned).toBe(expected);
+ });
+
+ it('should work with percentages', function () {
+ var returned = extractUnit('50%');
+ var expected = '%';
+
+ expect(returned).toBe(expected);
+ });
+ });
+
+ describe('tools.parseAngle', function() {
+ var parseAngle = tools.parseAngle;
+
+ it('calculates radians for degrees', function() {
+ var calculated = parseAngle('45deg');
+ var expected = 0.7853981633974483;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+
+ it('calculates radians for turns', function() {
+ var calculated = parseAngle('1turn');
+ var expected = 6.283185307179586;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+
+ it('calculates radians for gradians', function () {
+ var calculated = parseAngle('-32grad');
+ var expected = -0.5026548245743669;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+
@basecode Owner
basecode added a note

Other edge cases:

  • calculates radians for radians as a string
  • calculates radians for radians as a number

for radians as a string as in parseAngle('0.321512324') or as in parseAngle('0.321512341rad')?

Added tests for the following scenarios:

parseAngle('0.42131') parseAngle(0.42131) and parseAngle('0.42131rad')

@basecode Owner
basecode added a note

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ it('calculates radians for radians (string)', function () {
+ var calculated = parseAngle('-0.5026548245743669rad');
+ var expected = -0.5026548245743669;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+
+ it('calculates radians for radians', function () {
+ var calculated = parseAngle('-0.5026548245743669');
+ var expected = -0.5026548245743669;
+
+ expect(calculated).toBeCloseTo(expected, 10);
+ });
+ });
+
});
Something went wrong with that request. Please try again.