Re: Improved color support

[e...@eae.net]

On 2009/06/23 01:28:45, arv wrote:

Your last update was empty, I bet that wasn't intentional.

http://codereview.appspot.com/67158

[e...@eae.net]

Looks much better, one last comment though.

(btw, patch set 1 seems to have vanished making it harder to do patch to
patch diffs).

http://codereview.appspot.com/67158/diff/3001/4001
File trunk/excanvas.js (right):

http://codereview.appspot.com/67158/diff/3001/4001#newcode261
Line 261: aliceblue: '#F0F8FF',
In the interest of saving a few bytes, how about removing the leading
hash symbol from the color codes?

http://codereview.appspot.com/67158/diff/3001/4001#newcode477
Line 477: str = colorData[styleString] || styleString;
...and then changing this line to read

str = '#' + colorData[styleString] || styleString;

http://codereview.appspot.com/67158

[erik.ar...@gmail.com]

I think we should either keep it readable or small. If we go for
readable, which I've been convinced, we should then I think it is
cleaner to have the values of the object literal be valid CSS colors.

http://codereview.appspot.com/67158/diff/3001/4001
File trunk/excanvas.js (right):

http://codereview.appspot.com/67158/diff/3001/4001#newcode261
Line 261: aliceblue: '#F0F8FF',

On 2009/06/23 01:52:35, eae wrote:
> In the interest of saving a few bytes, how about removing the leading
hash
> symbol from the color codes?

With gzip this will not matter. If we add a string concat every time we
want to get a value it will add two object allocations.

http://codereview.appspot.com/67158/diff/3001/4001#newcode477
Line 477: str = colorData[styleString] || styleString;

On 2009/06/23 01:52:35, eae wrote:
> ...and then changing this line to read

> str = '#' + colorData[styleString] || styleString;

That would be

str = styleString in colorData ? '#' + colorData[styleString] :
styleString;

http://codereview.appspot.com/67158

[e...@eae.net]

LGTM

You've got me convinced, let's go for readable.

http://codereview.appspot.com/67158