[WIP] Add UX improvements to expression tree generator#3
Conversation
Agent-Logs-Url: https://github.com/psaegert/expression-tree-gen/sessions/9d244446-6338-4b90-b8b0-d1dc770c4140 Co-authored-by: psaegert <36567814+psaegert@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds UX controls to the expression tree generator for exporting the canvas, keyboard-driven generation, and enabling/disabling render animations.
Changes:
- Added Export PNG button with background mode selector (white vs transparent).
- Added Enter-key handler to trigger tree generation from the expression input.
- Added animation toggle and propagated an
animateflag through rendering; updated styling for new controls.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
index.html |
Adds Export PNG controls and Animate toggle to the UI. |
canvas.js |
Wires UI events (export, Enter-to-generate) and passes animate flag into rendering. |
tree.js |
Updates edge drawing and recursive rendering to honor an animate flag. |
app.css |
Adds wrapping/gap styling for the expanded button/control row and label styling. |
Comments suppressed due to low confidence (1)
tree.js:83
Node.drawEdgenow takes ananimateflag but has no default, and the implementation treats a missing value as falsy (non-animated). To preserve the previous default behavior and make the method safer for any future direct callers, consider defaulting the parameter totrue(or explicitly coercing undefined to true).
this.drawEdge = function (context, childNode, resolve, animate) {
const radius = this.getRadius(context)
const childRadius = childNode.getRadius(context)
context.strokeStyle = 'gray';
const dx = childNode.x - this.x
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var exportCanvas = document.createElement('canvas') | ||
| var exportContext = exportCanvas.getContext('2d') | ||
| exportCanvas.width = canvas.width | ||
| exportCanvas.height = canvas.height | ||
| exportContext.fillStyle = '#ffffff' | ||
| exportContext.fillRect(0, 0, exportCanvas.width, exportCanvas.height) | ||
| exportContext.drawImage(canvas, 0, 0) | ||
| dataURL = exportCanvas.toDataURL('image/png') |
There was a problem hiding this comment.
In exportCanvas(), the local var exportCanvas = document.createElement('canvas') shadows the function name. This makes stack traces/debugging harder and prevents any future self-reference/recursion. Rename the local variable (e.g., offscreenCanvas) to avoid identifier shadowing.
| var exportCanvas = document.createElement('canvas') | |
| var exportContext = exportCanvas.getContext('2d') | |
| exportCanvas.width = canvas.width | |
| exportCanvas.height = canvas.height | |
| exportContext.fillStyle = '#ffffff' | |
| exportContext.fillRect(0, 0, exportCanvas.width, exportCanvas.height) | |
| exportContext.drawImage(canvas, 0, 0) | |
| dataURL = exportCanvas.toDataURL('image/png') | |
| var offscreenCanvas = document.createElement('canvas') | |
| var exportContext = offscreenCanvas.getContext('2d') | |
| offscreenCanvas.width = canvas.width | |
| offscreenCanvas.height = canvas.height | |
| exportContext.fillStyle = '#ffffff' | |
| exportContext.fillRect(0, 0, offscreenCanvas.width, offscreenCanvas.height) | |
| exportContext.drawImage(canvas, 0, 0) | |
| dataURL = offscreenCanvas.toDataURL('image/png') |
| link.download = 'expression-tree.png' | ||
| link.click() |
There was a problem hiding this comment.
The download trigger uses link.click() on a detached <a> element. This is unreliable in some browsers (notably Safari) unless the link is temporarily appended to document.body and then removed. Consider appending/removing to improve cross-browser export behavior.
| link.download = 'expression-tree.png' | |
| link.click() | |
| link.download = 'expression-tree.png' | |
| document.body.appendChild(link) | |
| link.click() | |
| document.body.removeChild(link) |
/home/runner/work/expression-tree-gen/expression-tree-gen/index.html/home/runner/work/expression-tree-gen/expression-tree-gen/index.html/home/runner/work/expression-tree-gen/expression-tree-gen/canvas.js(export logic, Enter key triggers generate, animate flag passed to draw)/home/runner/work/expression-tree-gen/expression-tree-gen/tree.jsto honoranimateflag for edge drawing and recursive rendering/home/runner/work/expression-tree-gen/expression-tree-gen/app.css(button row wrapping + label styles)Original prompt
Goal
Add three UX improvements to the expression tree generator:
#expression-inputfield triggers the Generate button.Files involved
index.html— add UI controls (export button, bg-mode selector, animation toggle)canvas.js— wire up event handlers, implement export, handle Enter key, respect animation settingtree.js— makedrawEdgeAnimated/drawTreehonor an "animate" flagapp.css— minor styling for the new controls if neededRequired changes
1. Export button (
index.html,canvas.js)Add an Export PNG button to the
.buttonscontainer inindex.html. Next to it, add a control for choosing the background mode — a small<select>or a pair of radio buttons with options White (default) and Transparent.Example markup:
Implementation in
canvas.js:#export-bg-mode.#ffffff, thendrawImage(canvas, 0, 0)onto it, and export that viatoDataURL('image/png').toDataURL('image/png'). The currentclearCanvas()usesclearRect, which leaves the canvas transparent, so this will work as long as the CSS page background (the#canvas-containergrid pattern) does not bleed in — it won't, becausetoDataURLonly captures the canvas pixel data.<a>element withhref = dataURLanddownload = 'expression-tree.png', then.click()it.Important: Nodes already fill with white (
tree.jsline 101:context.fillStyle = 'white'; context.fill()). No change needed to node fill — it will remain white even when the page background is transparent. This satisfies the "nodes always filled white" requirement automatically.Edge case: if no tree has been generated yet (canvas empty), the export will produce an empty/transparent PNG. That's acceptable, but consider disabling the Export button until a tree exists, or just letting it produce an empty image. Simpler: always allow and don't worry about it.
2. Enter key to generate (
canvas.js)Add a
keydownlistener on#expression-input:3. Animation toggle (
index.html,canvas.js,tree.js)Add a checkbox next to the buttons:
Wire it through:
canvas.js, before callingdrawTree(root, c), readdocument.getElementById('animate-toggle').checkedand pass it intodrawTreeas a new parameter (defaulttruefor backward compatibility).tree.js, updatedrawTree(root, context, animate = true)to forwardanimateto child calls and intodrawEdge.Node.drawEdge(context, childNode, resolve, animate)to branch:animateistrue, calldrawEdgeAnimated(...)as today.animateisfalse, draw the edge synchronously with a singlebeginPath/moveTo/lineTo/stroke, then callresolve()immediately (still keep the async contract so the rest ofdrawTreeworks unchanged).Alternatively, bypass the promise entirely when not animating: in
drawTree, if!animate, use a synchronous traversal that draws edges + nodes withoutawait. Either approach is fine — prefer the simpler one (keeping the promise flow but resolving immediately).Styling notes (
app.css)The
.buttonsflex container already lays items in a row with 15px margin-right per child. The new controls should inherit that layout. You may need to tweak.buttonstoflex-wrap: wrapso the row doesn't overflow on narrow screens, and add a bit of vertical margin for wrapped rows:Label text color should match the existing
#e0e0e0used elsewhere in the contact section for readability on the dark background.Acceptance criteria
This pull request was created from Copilot chat.