-
-
Notifications
You must be signed in to change notification settings - Fork 307
Uploading for review for shape tool #1086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
this.ctx.restore(); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray edit
let [selectX, selectY] = this.imageCoordToCanvasCoord(this.selectX, this.selectY); | ||
this.drawSelectionBox(selectX, selectY, this.selectWidth * this.zoomLevel, this.selectHeight * this.zoomLevel, this.uiColor, 8 * this.zoomLevel, 0); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't have blank lines here
|
||
if (this.tools.shape && this.tools.shape.shapes) { | ||
for (let shape of this.tools.shape.shapes) { | ||
this.tools.shape.drawShape(shape); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shapes should either direct draw, or act as sub-layers on a layer. Doesn't really make sense to render in the main function here like this
} | ||
|
||
onMouseUp(e) { | ||
if (this.isDrawing && (this.shape === 'rectangle' || this.shape === 'circle')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==
not ===
, but also why is this check here at all?
|
||
// Only add if there's actual size | ||
if (Math.abs(this.currentX - this.startX) > 1 || Math.abs(this.currentY - this.startY) > 1) { | ||
this.shapes.push({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format wonk
let w = shape.width * zoom; | ||
let h = shape.height * zoom; | ||
ctx.rect(x, y, w, h); | ||
} else if (shape.type === 'circle') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline after a }
last commit for tonight lol too tired
alright i updated it quite a lot, i think i got all the errors and wonky formatting??? using a sub layer now, removed the check it was for accidental clicks but really if it's inside the canvas we don't need that and outside the canvas doesn't matter I can imagine there is more work to be done but for now, it's late. |
this.startY = 0; | ||
this.currentX = 0; | ||
this.currentY = 0; | ||
this.shapes = []; // Track shapes for undo functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be tracked
this.currentX = 0; | ||
this.currentY = 0; | ||
this.shapes = []; // Track shapes for undo functionality | ||
this.shapesLayer = null; // Sub-layer for storing shapes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tool is not associated with any layer, it applies to whichever layer is currently active
Was a bit me and a bit of help on some stuff from LLM, I've always been honest with that. Just let me know what parts might be out of your standards and I'll do my best to bring them into line.