Sky color chooser example

A simple Sky color chooser example. The color-picker is
two components: color-wheel, and color-picker which adds
a div that displays the selected color. Data binding is
used to connect the color-wheel's "color" property to the
sample div's background-color.

BUG=
R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/54b1ffda2b75998fd63567e4637d0c07441f629b

[hansmuller1, 2015-01-31 00:36:14]

hansmuller@google.com changed reviewers:
+ abarth@chromium.org, esprehn@chromium.org

[hansmuller1, 2015-01-31 00:36:39]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-01-31 00:36:39]

PTAL

[abarth-chromium, 2015-01-31 03:20:19]

Looks pretty good.  Below are just some style comments.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
File sky/examples/color/color-picker.sky (right):

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:2: <sky>
Please add a copyright block.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:4: <import
src="../../framework/sky-element/sky-element.sky" as="SkyElement" />
We usually include the framework as a root-relative path from the examples:

<import src="/sky/framework/sky-box.sky"/>

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:7: <template>
Two space indent please.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:9: .color-sample {
.color-sample  ->   #color-sample

We usually use IDs inside components.  IDs are scoped to components, so there's
no risk of a naming collision.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:28: <color-picker-root />
I'd probably put the color-picker component in its own sky file and have the
root file just import it an have the <color-picker /> element.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
File sky/examples/color/color-wheel.sky (right):

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:1: <import
src="../../framework/sky-element/sky-element.sky" as="SkyElement" />
Please add a copyright block.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:7: on-pointerup="handlePointerUp">
You should listen for on-pointercancel as well.  You get that event instead of
pointerup when the finger moves off the screen.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:12: }
This style doesn't appear to apply to anything.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:12: }
This style doesn't appear to apply to anything.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:13: .color-wheel-img {
There's no need for this class.  You can just use

img { ... }

The style is scoped to this component so there's no risk of styling other
things.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:92: if (this.pointerIsDown)
You'll never get a pointermove event unless the pointer is down, so you don't
need the pointerIsDown member variable.

[esprehn, 2015-02-01 12:28:43]

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
File sky/examples/color/color-wheel.sky (right):

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:30: this.b = b;
Object.preventExtensions(this)

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:37: function HSVtoRGB(h, s, v) {
hsvToRgb()

Treat acronyms like words.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:50: case 5: r = v, g = p, b = q; break;
All these single letter vars are pretty hard to read, also all the assignments
on one line.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:58: class ColorWheel {
This should just be a function, it has no state.

[hansmuller1, 2015-02-02 16:32:31]

Thanks for all of the feedback; I've made the suggested revisions.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
File sky/examples/color/color-picker.sky (right):

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:2: <sky>
On 2015/01/31 03:20:18, abarth wrote:
> Please add a copyright block.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:4: <import
src="../../framework/sky-element/sky-element.sky" as="SkyElement" />
On 2015/01/31 03:20:18, abarth wrote:
> We usually include the framework as a root-relative path from the examples:
> 
> <import src="/sky/framework/sky-box.sky"/>

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:7: <template>
On 2015/01/31 03:20:18, abarth wrote:
> Two space indent please.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-pic...
sky/examples/color/color-picker.sky:9: .color-sample {
On 2015/01/31 03:20:18, abarth wrote:
> .color-sample  ->   #color-sample
> 
> We usually use IDs inside components.  IDs are scoped to components, so
there's
> no risk of a naming collision.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
File sky/examples/color/color-wheel.sky (right):

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:1: <import
src="../../framework/sky-element/sky-element.sky" as="SkyElement" />
On 2015/01/31 03:20:18, abarth wrote:
> Please add a copyright block.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:7: on-pointerup="handlePointerUp">
On 2015/01/31 03:20:18, abarth wrote:
> You should listen for on-pointercancel as well.  You get that event instead of
> pointerup when the finger moves off the screen.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:12: }
On 2015/01/31 03:20:18, abarth wrote:
> This style doesn't appear to apply to anything.

Oops. Removed that.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:13: .color-wheel-img {
On 2015/01/31 03:20:18, abarth wrote:
> There's no need for this class.  You can just use
> 
> img { ... }
> 
> The style is scoped to this component so there's no risk of styling other
> things.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:30: this.b = b;
On 2015/02/01 12:28:43, esprehn wrote:
> Object.preventExtensions(this)

I got rid of this class, since it ended up with the same just-a-function purpose
as ColorWheel.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:37: function HSVtoRGB(h, s, v) {
On 2015/02/01 12:28:43, esprehn wrote:
> hsvToRgb()
> 
> Treat acronyms like words.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:50: case 5: r = v, g = p, b = q; break;
On 2015/02/01 12:28:43, esprehn wrote:
> All these single letter vars are pretty hard to read, also all the assignments
> on one line.

I removed the re-declaration of r,g,b (and oops). The arithmetic for RGB to HSV
is intrinsically hard to read, I don't think making up long variable names for
the intermediate results would help.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:58: class ColorWheel {
On 2015/02/01 12:28:43, esprehn wrote:
> This should just be a function, it has no state.

Done.

https://codereview.chromium.org/872993006/diff/1/sky/examples/color/color-whe...
sky/examples/color/color-wheel.sky:92: if (this.pointerIsDown)
On 2015/01/31 03:20:18, abarth wrote:
> You'll never get a pointermove event unless the pointer is down, so you don't
> need the pointerIsDown member variable.

Done.

[abarth-chromium, 2015-02-02 16:53:55]

LGTM

Please add a description that explains what's in this CL.  It doesn't have to be
anything fancy, but something more than "bug="

https://codereview.chromium.org/872993006/diff/20001/sky/examples/color/color...
File sky/examples/color/color-picker.sky (right):

https://codereview.chromium.org/872993006/diff/20001/sky/examples/color/color...
sky/examples/color/color-picker.sky:22: </sky-box>
These should be indented two more spaces (so sky-box lines up with style)

https://codereview.chromium.org/872993006/diff/20001/sky/examples/color/color...
sky/examples/color/color-picker.sky:29: }).register();
Typically we'd write:

module.exports = class extends SkyElement {
...
}.register();

In theory, element registrations aren't available in other modules unless
exported, but we haven't implemented that yet.  :)

[hansmuller1, 2015-02-02 17:01:36]

Thanks for the super-fast feedback!

https://codereview.chromium.org/872993006/diff/20001/sky/examples/color/color...
File sky/examples/color/color-picker.sky (right):

https://codereview.chromium.org/872993006/diff/20001/sky/examples/color/color...
sky/examples/color/color-picker.sky:22: </sky-box>
On 2015/02/02 16:53:55, abarth wrote:
> These should be indented two more spaces (so sky-box lines up with style)

Done.

https://codereview.chromium.org/872993006/diff/20001/sky/examples/color/color...
sky/examples/color/color-picker.sky:29: }).register();
On 2015/02/02 16:53:55, abarth wrote:
> Typically we'd write:
> 
> module.exports = class extends SkyElement {
> ...
> }.register();
> 
> In theory, element registrations aren't available in other modules unless
> exported, but we haven't implemented that yet.  :)

Done.

[hansmuller, 2015-02-02 17:01:58]

Committed patchset #3 (id:40001) manually as
54b1ffda2b75998fd63567e4637d0c07441f629b (presubmit successful).