Add designer-selection element (issue 877243005 by justinfagnani@google.com)

31 views
Skip to first unread message

justin...@google.com

unread,
Feb 4, 2015, 7:17:04 PM2/4/15
to ia...@google.com, polymer-d...@googlegroups.com
Reviewers: Hixie,

Description:
Add designer-selection element

BUG=

Please review this at https://codereview.chromium.org/877243005/

Base URL: https://github.com/PolymerLabs/designer2.git@master

Affected files (+313, -0 lines):
A .gitignore
A bower.json
A codereview.settings
A elements/designer-selection/README.md
A elements/designer-selection/demo.html
A elements/designer-selection/designer-selection.html


justin...@google.com

unread,
Feb 4, 2015, 7:17:56 PM2/4/15
to im...@google.com, polymer-d...@googlegroups.com
Message has been deleted

im...@google.com

unread,
Feb 4, 2015, 10:18:34 PM2/4/15
to justin...@google.com, polymer-d...@googlegroups.com

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-selection/designer-selection.html
File elements/designer-selection/designer-selection.html (right):

https://codereview.chromium.org/877243005/diff/20001/elements/designer-selection/designer-selection.html#newcode23
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-selection/designer-selection.html#newcode94
elements/designer-selection/designer-selection.html:94: };
Yay!

https://codereview.chromium.org/877243005/diff/20001/elements/designer-selection/designer-selection.html#newcode127
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-selection/designer-selection.html#newcode131
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-selection/designer-selection.html#newcode167
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-selection/designer-selection.html#newcode176
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-selection/designer-selection.html#newcode192
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-selection/designer-selection.html#newcode196
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-selection/designer-selection.html#newcode201
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-selection/designer-selection.html#newcode214
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)

https://codereview.chromium.org/877243005/
Reply all
Reply to author
Forward
0 new messages