Add designer-stage (issue 881243003 by justinfagnani@google.com)

18 views
Skip to first unread message

justin...@google.com

unread,
Feb 4, 2015, 7:46:32 PM2/4/15
to im...@google.com, polymer-d...@googlegroups.com
Reviewers: imac_google.com,

Message:
FYI: This has a little bit of demo content for testing until I get some
content
loading functionality ready.

Description:
Add designer-stage

BUG=

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

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

Affected files (+308, -0 lines):
A elements/designer-stage/demo.html
A elements/designer-stage/designer-stage.html
A elements/designer-stage/frame.html


im...@google.com

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

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo.html
File elements/designer-stage/demo.html (right):

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo.html#newcode5
elements/designer-stage/demo.html:5: -->
Go w/ the big license blurb (e.g.
https://github.com/Polymer/wct-local/blob/master/lib/plugin.js#L1-9):

* It points to the authors/contributors/patents lists.
* You don't need a separate LICENSE file w/ this guy.
* @license makes various tools happy (uglify will preserve it).

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo.html#newcode10
elements/designer-stage/demo.html:10: body {
Probably want html, body { here so you have height: 100% on html too

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html
File elements/designer-stage/designer-stage.html (right):

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode11
elements/designer-stage/designer-stage.html:11: <link rel="import"
href="../designer-selection/designer-selection.html">
I think you forgot to include this in the patch

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode13
elements/designer-stage/designer-stage.html:13: <body>
IMO going w/ the `html`-less and `body`-less style makes way more sense
for imports:

* It makes it pretty clear that this file is not to be used directly as
a master document.
* Less boilerplate per import.
* Also, it's just the norm for all our import.

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode42
elements/designer-stage/designer-stage.html:42: }
You may want pointer-events: none; on this guy (but it's not clear to me
w/o designer-selection's source)

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode68
elements/designer-stage/designer-stage.html:68: frame.onload =
function() {
Hey, use `addEventListener` here! I don't have a rational reason other
than the old style feels wrong

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode68
elements/designer-stage/designer-stage.html:68: frame.onload =
function() {
Seems like there's a potential race here too: couldn't the frame have
loaded by now (e.g. polyfill land)?

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode73
elements/designer-stage/designer-stage.html:73: };
Don't forget to handle the `error` event!

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode83
elements/designer-stage/designer-stage.html:83: },
Are you aiming for a UUID here, or just something reasonably unique?
Might want to use crypto.getRandomValues() when it's available (e.g.
everything but IE 10)

(being on the same machine makes RNG attacks a bit more real, I think)

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode87
elements/designer-stage/designer-stage.html:87: throw new Error('Invalid
token');
Stick the name of the token in that error message so it's more
actionable (log injection concerns be damned, IMO). Or maybe console.log
it in addition to the throw

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode97
elements/designer-stage/designer-stage.html:97: }
As this grows, consider moving to a lookup table

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode110
elements/designer-stage/designer-stage.html:110: style.height =
bounds.height;
Probably want to clamp these to stay within the stage (or maybe use
overflow: hidden to accomplish that)

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode116
elements/designer-stage/designer-stage.html:116: var bounds =
data.bounds;
`data` can be null here

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode120
elements/designer-stage/designer-stage.html:120: selection.directions =
ResizeDirection.width_height;
Where is `ResizeDirection` coming from?

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode125
elements/designer-stage/designer-stage.html:125: // Possibly implement
message batching or extension.
Damn right you'll factor that out!

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode143
elements/designer-stage/designer-stage.html:143: }, '*');
Might be good to use an actual origin. What if an element
`window.location`s on you?

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode151
elements/designer-stage/designer-stage.html:151: message_type:
'selectElement',
IMO you should do the dreaded ##ms debounce here, otherwise every time
someone clicks the element is going to briefly select.

Aka, have selection begin after 300ms, OR the mouse moved 10px or so.

https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/designer-stage.html#newcode153
elements/designer-stage/designer-stage.html:153: y: e.clientY
You should probably translate these relative to `frame`. (Also, I could
have sworn there was a coordinate translation helper somewhere in the
depths of the DOM API, but I can't find it...)

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