Add designer-selection element

Add designer-selection element

BUG=

[justinfagnani, 2015-02-05 00:17:02]

justinfagnani@google.com changed reviewers:
+ ianh@google.com

[justinfagnani, 2015-02-05 00:17:54]

justinfagnani@google.com changed reviewers:
+ imac@google.com
- ianh@google.com

[imac, 2015-02-05 03:18:33]

https://codereview.chromium.org/877243005/diff/20001/bower.json
File bower.json (right):

https://codereview.chromium.org/877243005/diff/20001/bower.json#newcode7
bower.json:7: "neoprene": "https://github.com/PolymerLabs/neoprene.git",
PolymerLabs/neoprene is shorthand for this; consistency!

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
File elements/designer-selection/designer-selection.html (right):

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:23: border: solid 1px #88f;
do you want border-box here?

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:94: };
Yay!

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:127: var bottom_right =
ResizeDirection.bottom_right = new ResizeDirection('bottom_right', 1.0, 1.0);
these should probably be ALL_CAPS (constants), or maybe camelCase

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:131: })();
consider moving ResizeDirection out to a separate import/script

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:167: if (this._onMove !=
null) {
this feels like it should be an assert

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:176:
this.$.bounds.style.cursor = 'move';
wouldn't you want the cursor to always be this to hint to the user that they can
drag?

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:192: _handleDown:
function(e) {
_resizeDown/_resizeMove would be clearer, I think

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:196: var boundsBounds =
this.$.bounds.getBoundingClientRect();
http://vimeo.com/87712359

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:201: : boundsBounds.left +
(e.clientX - handleBounds.left);
I'm confused, why does the position of the handle matter?

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:214: direction.resizesLeft()
? this.offsetWidth + this.offsetLeft - x :
Rather than having to calculate offsetLeft/Top each frame, you might want to
just hold onto it when the move starts, and then just move relative to the mouse
position on mousedown. (also accumulation errors)

[powei, 2015-02-05 03:29:36]

powei@chromium.org changed reviewers:
+ powei@chromium.org

[powei, 2015-02-05 03:29:37]

Sorry for the drive-by noise, but this review is being posted on to the
public-facing polymer-dev@ group.  Is this intentional?  

I've removed the CC: polymer-dev+review@googlegroups.com for now. Please add it
back if the intention was to have the review posted on that group.  

Same for the other issue here:
https://codereview.chromium.org/881243003/

[justinfagnani, 2015-02-05 19:35:18]

On 2015/02/05 03:29:37, powei wrote:
> Sorry for the drive-by noise, but this review is being posted on to the
> public-facing polymer-dev@ group.  Is this intentional?  
> 
> I've removed the CC: mailto:polymer-dev+review@googlegroups.com for now.
Please add it
> back if the intention was to have the review posted on that group.  
> 
> Same for the other issue here:
> https://codereview.chromium.org/881243003/

Thanks powei, I had figured -dev was the team list.

[justinfagnani, 2015-02-05 20:15:55]

https://codereview.chromium.org/877243005/diff/20001/bower.json
File bower.json (right):

https://codereview.chromium.org/877243005/diff/20001/bower.json#newcode7
bower.json:7: "neoprene": "https://github.com/PolymerLabs/neoprene.git",
On 2015/02/05 03:18:33, imac wrote:
> PolymerLabs/neoprene is shorthand for this; consistency!

Done.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
File elements/designer-selection/designer-selection.html (right):

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:23: border: solid 1px #88f;
On 2015/02/05 03:18:33, imac wrote:
> do you want border-box here?

Done.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:94: };
On 2015/02/05 03:18:33, imac wrote:
> Yay!

Done.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:127: var bottom_right =
ResizeDirection.bottom_right = new ResizeDirection('bottom_right', 1.0, 1.0);
On 2015/02/05 03:18:33, imac wrote:
> these should probably be ALL_CAPS (constants), or maybe camelCase

I'll do camel case, I don't find ALL_CAPS to convey very useful information.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:131: })();
On 2015/02/05 03:18:33, imac wrote:
> consider moving ResizeDirection out to a separate import/script

Done.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:167: if (this._onMove !=
null) {
On 2015/02/05 03:18:33, imac wrote:
> this feels like it should be an assert

Done.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:176:
this.$.bounds.style.cursor = 'move';
On 2015/02/05 03:18:33, imac wrote:
> wouldn't you want the cursor to always be this to hint to the user that they
can
> drag?

I'm not totally sure, you should also be able to click to reselect what's under
the mouse. We can evolve this.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:192: _handleDown:
function(e) {
On 2015/02/05 03:18:33, imac wrote:
> _resizeDown/_resizeMove would be clearer, I think

_resizeHandleDown is the most clear!

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:196: var boundsBounds =
this.$.bounds.getBoundingClientRect();
On 2015/02/05 03:18:33, imac wrote:
> http://vimeo.com/87712359

Acknowledged.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:201: : boundsBounds.left +
(e.clientX - handleBounds.left);
On 2015/02/05 03:18:33, imac wrote:
> I'm confused, why does the position of the handle matter?

This prevents a jump on first move by the amount the cursor is offset from the
handle's top-left. I'm pretty sure I'll need to revisit some of the calcs to
make them work in non-full bleed modes.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:214: direction.resizesLeft()
? this.offsetWidth + this.offsetLeft - x :
On 2015/02/05 03:18:33, imac wrote:
> Rather than having to calculate offsetLeft/Top each frame, you might want to
> just hold onto it when the move starts, and then just move relative to the
mouse
> position on mousedown. (also accumulation errors)

Yeah, this.offsetWidth + this.offsetLeft is what I need because offsetRight
doesn't exist. Right now anything I need to remember between callbacks has to be
an instance member. I'd like to fix that so I can use scopes instead, so let me
defer this for now.

[justinfagnani, 2015-02-05 22:16:21]

justinfagnani@google.com changed reviewers:
- powei@chromium.org

[imac, 2015-02-06 20:26:31]

lgtm

https://codereview.chromium.org/877243005/diff/20001/bower.json
File bower.json (right):

https://codereview.chromium.org/877243005/diff/20001/bower.json#newcode7
bower.json:7: "neoprene": "https://github.com/PolymerLabs/neoprene.git",
On 2015/02/05 20:15:54, justinfagnani wrote:
> On 2015/02/05 03:18:33, imac wrote:
> > PolymerLabs/neoprene is shorthand for this; consistency!
> 
> Done.

polymer & webcomponentsjs too

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
File elements/designer-selection/designer-selection.html (right):

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:192: _handleDown:
function(e) {
On 2015/02/05 20:15:54, justinfagnani wrote:
> On 2015/02/05 03:18:33, imac wrote:
> > _resizeDown/_resizeMove would be clearer, I think
> 
> _resizeHandleDown is the most clear!

Most clearerer!

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:214: direction.resizesLeft()
? this.offsetWidth + this.offsetLeft - x :
On 2015/02/05 20:15:54, justinfagnani wrote:
> On 2015/02/05 03:18:33, imac wrote:
> > Rather than having to calculate offsetLeft/Top each frame, you might want to
> > just hold onto it when the move starts, and then just move relative to the
> mouse
> > position on mousedown. (also accumulation errors)
> 
> Yeah, this.offsetWidth + this.offsetLeft is what I need because offsetRight
> doesn't exist. Right now anything I need to remember between callbacks has to
be
> an instance member. I'd like to fix that so I can use scopes instead, so let
me
> defer this for now.

Hmm, I mean that you shouldn't need to refer to this.offset* _at all_ from
within the move function. The thought is:

* in _resizeHandleDown, you store e.clientX/Y
* in _resizeHandleMove, you just emit the delta between the current e.clientX/Y
and the value stored.
  * or keep emitting the calculated size as you are, but you can calculate it
from the original position, not the iterative one

This saves you from a few potential sources of error:

* If someone upstream does something whacky (resizing the element while
resizing, bad math, whatever), the resizer isn't going to flip its shit
* You don't have to worry about accumulation errors (constantly adding offsets;
though integers, so maybe not a concern).
* What happens when an element resizes down to 0 width or height?
* If you just pass the deltas up, the code managing the resize can do whatever
it wants. And because it's the delta from the original position, you're not
dependent on the state from the previous move events

[justinfagnani, 2015-02-06 23:46:15]

New patchsets have been uploaded after l-g-t-m from imac@google.com

[justinfagnani, 2015-02-06 23:46:39]

Thanks!

https://codereview.chromium.org/877243005/diff/20001/bower.json
File bower.json (right):

https://codereview.chromium.org/877243005/diff/20001/bower.json#newcode7
bower.json:7: "neoprene": "https://github.com/PolymerLabs/neoprene.git",
On 2015/02/06 20:26:31, imac wrote:
> On 2015/02/05 20:15:54, justinfagnani wrote:
> > On 2015/02/05 03:18:33, imac wrote:
> > > PolymerLabs/neoprene is shorthand for this; consistency!
> > 
> > Done.
> 
> polymer & webcomponentsjs too

Done.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
File elements/designer-selection/designer-selection.html (right):

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:192: _handleDown:
function(e) {
On 2015/02/06 20:26:31, imac wrote:
> On 2015/02/05 20:15:54, justinfagnani wrote:
> > On 2015/02/05 03:18:33, imac wrote:
> > > _resizeDown/_resizeMove would be clearer, I think
> > 
> > _resizeHandleDown is the most clear!
> 
> Most clearerer!

Acknowledged.

https://codereview.chromium.org/877243005/diff/20001/elements/designer-select...
elements/designer-selection/designer-selection.html:214: direction.resizesLeft()
? this.offsetWidth + this.offsetLeft - x :
On 2015/02/06 20:26:31, imac wrote:
> On 2015/02/05 20:15:54, justinfagnani wrote:
> > On 2015/02/05 03:18:33, imac wrote:
> > > Rather than having to calculate offsetLeft/Top each frame, you might want
to
> > > just hold onto it when the move starts, and then just move relative to the
> > mouse
> > > position on mousedown. (also accumulation errors)
> > 
> > Yeah, this.offsetWidth + this.offsetLeft is what I need because offsetRight
> > doesn't exist. Right now anything I need to remember between callbacks has
to
> be
> > an instance member. I'd like to fix that so I can use scopes instead, so let
> me
> > defer this for now.
> 
> Hmm, I mean that you shouldn't need to refer to this.offset* _at all_ from
> within the move function. The thought is:
> 
> * in _resizeHandleDown, you store e.clientX/Y
> * in _resizeHandleMove, you just emit the delta between the current
e.clientX/Y
> and the value stored.
>   * or keep emitting the calculated size as you are, but you can calculate it
> from the original position, not the iterative one
> 
> This saves you from a few potential sources of error:
> 
> * If someone upstream does something whacky (resizing the element while
> resizing, bad math, whatever), the resizer isn't going to flip its shit
> * You don't have to worry about accumulation errors (constantly adding
offsets;
> though integers, so maybe not a concern).
> * What happens when an element resizes down to 0 width or height?
> * If you just pass the deltas up, the code managing the resize can do whatever
> it wants. And because it's the delta from the original position, you're not
> dependent on the state from the previous move events

Yes, but I want to restructure this so I don't have to keep adding instance
members to remember values between event callbacks. I'll revisit very soon.