css-shapes-editor icon indicating copy to clipboard operation
css-shapes-editor copied to clipboard

Address Code Review by NJ

Open oslego opened this issue 11 years ago • 0 comments

CSSUtils.getContentBoxOf(): there’s a TODO here about not handling cases other than content-box. Will that affect anything?

~~Editor.setupEditorHolder(): the way it calls itself back is a bit confusing - it seems like it would be clearer to just reverse the condition (i.e. if (!this.holder), create the element, and then do the other stuff after the if).~~

~~Editor.setupShapeDecoration(): the check for whether the path isn't already an array could just be !Array.isArray(path), I think.~~

~~Editor.turnOnFreeTransform()/turnOffFreeTransform(): could just reverse the conditions~~

PolygonEditor/CircleEditor/EllipseEditor.setup(): it looks like the event listeners added at the end of these functions are never removed.

PolygonEditor.inferShapeFromElement(): there’s a TODO here about not dealing with non-px units - is that a potential problem (e.g. might people want to use shapes for boxes that are sized in ems/rems)?

~~PolygonEditor.onMouseDown(): when checking the vertex index, don’t need to parseInt() since you’ve already determined that the type is “number” (similar code you have later doesn’t do the parseInt())~~

EllipseEditor.parseShape(): there’s a TODO here for handling centers other than the default 50% 50% - is that something we’ll be likely to encounter?

oslego avatar Jun 12 '14 08:06 oslego