fix(comments): popover position, rubber-band inclusion, Escape to close #35

Merged

AhmedHanafy725 merged 1 commit from development_comment_ux_fixes into development

2026-04-21 15:04:47 +00:00

AhmedHanafy725 commented

2026-04-21 14:26:24 +00:00

Member

Summary

Three small UX fixes on the comment tool, closing #32, #33, #34.

Closes #32
Closes #33
Closes #34

Changes

crates/hero_whiteboard_ui/static/web/js/whiteboard/comments.js
- positionPopover: stopped double-counting the stage transform. getAbsolutePosition already returns stage-container pixels, so use stageBox.left/top + markerPos.x/y directly instead of multiplying by stage.scaleX() and adding stage.x/y on top. Fixes the popover landing far from the marker at zoom-out.
- Added installEscHandler / removeEscHandler helpers that attach a document-level keydown listener for Escape while any comment popover is open; hidePopover removes it. Both the thread popover and the new-comment popover now dismiss on Esc.
crates/hero_whiteboard_ui/static/web/js/whiteboard/tools.js
- Rubber-band onMouseUp now also walks layer.find('.comment-marker') after the .object pass, so comment markers intersecting the rubber band are included in the transformer selection and move with a group drag.

Test Results

cargo check --workspace: pass
cargo clippy --workspace -- -D warnings: pass
cargo fmt --check: pass

Manual verification

Zoom out, click a comment marker — popover appears next to the marker, not off in the corner.
Rubber-band over a region containing a comment + other shapes, drag the selection — the comment moves with the rest.
Click a comment marker to open the thread popover, press Esc — popover closes.
Create a new comment, press Esc — new-comment popover closes.

## Summary Three small UX fixes on the comment tool, closing #32, #33, #34. ## Related Issues - Closes https://forge.ourworld.tf/lhumina_code/hero_whiteboard/issues/32 - Closes https://forge.ourworld.tf/lhumina_code/hero_whiteboard/issues/33 - Closes https://forge.ourworld.tf/lhumina_code/hero_whiteboard/issues/34 ## Changes - `crates/hero_whiteboard_ui/static/web/js/whiteboard/comments.js` - `positionPopover`: stopped double-counting the stage transform. `getAbsolutePosition` already returns stage-container pixels, so use `stageBox.left/top + markerPos.x/y` directly instead of multiplying by `stage.scaleX()` and adding `stage.x/y` on top. Fixes the popover landing far from the marker at zoom-out. - Added `installEscHandler` / `removeEscHandler` helpers that attach a document-level keydown listener for Escape while any comment popover is open; `hidePopover` removes it. Both the thread popover and the new-comment popover now dismiss on Esc. - `crates/hero_whiteboard_ui/static/web/js/whiteboard/tools.js` - Rubber-band `onMouseUp` now also walks `layer.find('.comment-marker')` after the `.object` pass, so comment markers intersecting the rubber band are included in the transformer selection and move with a group drag. ## Test Results - `cargo check --workspace`: pass - `cargo clippy --workspace -- -D warnings`: pass - `cargo fmt --check`: pass ## Manual verification - [ ] Zoom out, click a comment marker — popover appears next to the marker, not off in the corner. - [ ] Rubber-band over a region containing a comment + other shapes, drag the selection — the comment moves with the rest. - [ ] Click a comment marker to open the thread popover, press Esc — popover closes. - [ ] Create a new comment, press Esc — new-comment popover closes.

AhmedHanafy725 added 1 commit

2026-04-21 14:26:24 +00:00

fix(comments): popover position, rubber-band inclusion, Escape to close

CI / build (pull_request) Successful in 2m9s

Details

ad58bb4153

Three small UX fixes on the comment tool:

- positionPopover was double-counting the stage transform (markerPos from
  getAbsolutePosition is already stage-container pixels; the code then
  multiplied by scaleX and added stage.x/y). At zoom-out the popover
  landed far from its marker. Drop the extra terms.
- Rubber-band selection in tools.js iterated only '.object' nodes, so
  comment markers (name 'comment-marker') were never picked up; a group
  drag left them behind. Also walk '.comment-marker' on mouse up.
- No Escape handler on the thread popover. Added a document-level
  keydown handler installed when either popover opens and removed in
  hidePopover, so Esc now dismisses both the thread and new-comment
  popovers.

#32
#33
#34