PR_26179_CHARLIE_025-sprites-basic-drawing#261
Conversation
ToolboxAid
changed the base branch from
PR_26179_CHARLIE_024-sprites-canvas-grid
to
main
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19abdfd874
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .sprite-canvas-cell.is-painted { | ||
| background: var(--text) |
There was a problem hiding this comment.
The new drawing code only makes painted pixels visible through this .is-painted rule, but /toolbox/sprites/index.html links only assets/theme-v2/css/theme.css, and theme.css does not import gamefoundrystudio.css (repo-wide search also found no runtime import of this file). In the actual Sprite Creator page, clicking Pencil/Fill will add the class and update the count, but the pixel background will not change, so the core drawing behavior is effectively invisible to users.
Useful? React with 👍 / 👎.
|
|
||
| grid.replaceChildren(); | ||
| editorState.gridSize = size; | ||
| editorState.paintedPixels.clear(); |
There was a problem hiding this comment.
When a user has painted pixels and clicks the already-active 16x16/32x32 size button, the click still calls setGridSize with the same size, and this clear drops all session pixels and rebuilds the grid. Since the selected display-mode control remains enabled, reselecting the current mode should be a no-op or preserve the draft; otherwise an accidental click loses unsaved work.
Useful? React with 👍 / 👎.
| editorState.paintedPixels.delete(key); | ||
| cell.classList.remove("is-painted"); | ||
| } else { | ||
| editorState.paintedPixels.add(key); | ||
| cell.classList.add("is-painted"); |
There was a problem hiding this comment.
For screen-reader users, painting or erasing a grid cell only changes the CSS class and draft count; the cell's accessible name/state remains just Pixel row ..., column ..., so navigating the grid cannot distinguish painted pixels from empty ones. When adding drawing state, update each affected cell's label or an ARIA state such as aria-selected/aria-pressed in the paint/erase/fill paths so non-visual users can edit the sprite.
Useful? React with 👍 / 👎.
| } else { | ||
| editorState.paintedPixels.add(key); | ||
| cell.classList.add("is-painted"); |
There was a problem hiding this comment.
After the Fill button is clicked, the toolbar and status leave fill as the active tool, but clicking canvas cells still follows this generic non-eraser branch and paints only the single clicked pixel. For example, if Fill is selected and the grid is then reset by changing canvas size, the UI still says Fill is active, yet the next canvas click does not fill the grid; either Fill should not remain an active tool after its button action or this path should invoke the fill behavior.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
node --check assets/toolbox/sprites/js/index.jsPASSnode --check dev/tests/playwright/tools/SpritesToolShell.spec.mjsPASSgit diff --check ...PASSnpx playwright test dev/tests/playwright/tools/SpritesToolShell.spec.mjs --workers=1 --reporter=list --output=<temp>PASS, 1 test passedArtifact
dev/workspace/zip/PR_26179_CHARLIE_025-sprites-basic-drawing_delta.zip