Upgrade question plugin: extract module, add resize, verse context popover, and bug fixes#128
Open
hendriebeats wants to merge 9 commits into
Open
Upgrade question plugin: extract module, add resize, verse context popover, and bug fixes#128hendriebeats wants to merge 9 commits into
hendriebeats wants to merge 9 commits into
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Editor.tsx had grown to ~1700 lines with a large portion dedicated entirely to the question feature. Following the existing pattern of OtherCursorPlugin.ts and QuestionHighlightPlugin.ts, extract all question-specific code into its own module. Moved to QuestionPlugin.ts: - QuestionMapItem interface and Dictionary type - newQuestionNode / newQuestionAnswerNode factory functions - QuestionsView, QuestionView, QuestionAnswerView node views - questionReferenceMarkView and referenceToMarkView mark views - zIndices/zIndexCounter state and questionPopup function - questionMarkPlugin factory (with questionMarkWidget helper) - addQuestion, deleteQuestionSelection, deleteAnswerSelection, removeQuestion commands Editor.tsx retains all wiring: plugin registration, nodeViews/markViews entries, questionMap property, and addQuestionCommand method.
- Clicking the verse reference label on a question node now shows a popover with surrounding passage context: one verse before and after the highlighted selection, with the referenced text highlighted in yellow and dimmed neighboring verses for orientation. - Switch question popup to `position: fixed` so it stays in the viewport rather than drifting with the scrolling editor container. - Clamp popup within viewport on open and on window resize, shrinking width/height if the viewport is smaller than the popup. - Constrain drag/resize to use `window.innerHeight` instead of `document.body.scrollHeight` so bounds are always viewport-relative. - Increase minimum resize width from 200 px to 250 px. - Extract `closePopup` helper to de-duplicate destroy/remove logic and ensure the resize listener is always cleaned up on close. - Add `cursor: default; user-select: none` to verse reference label and `position: relative` to `.popup` content area (needed for absolute popover anchoring). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Convert close button and question-mark widget from custom/div elements to <button> so they have an implicit ARIA role and are keyboard-reachable (Enter/Space); add aria-label to both and aria-hidden to decorative icons. - Add CSS reset (border/padding: 0/none) to .closer and .questionMark so the new <button> elements look identical to before. - Fix QuestionView: assign this.questionMap unconditionally before the registration guard, so update() can sync the popup editor after undo/redo. - Add contenteditable="false" to noQuestionsText so ProseMirror does not try to manage it as document content. - Replace two innerHTML+window.base template literals with safe DOM API calls (createElement + .src) to eliminate latent XSS surface. - Remove duplicate undo/redo import, dead referenceToMarkView code (empty handlers, commented-out bodies, unused qselector variable), and self-evident inline comments. - Remove commented-out CSS rules and a duplicate margin-right declaration in .questionMark; trim excess blank lines in base.css. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bug 1 — clampToViewport NaN: the drag handler writes pop.style.left as "calc(924px)", so parseFloat(pop.style.left) returns NaN and the clamp is silently skipped. Switch to reading position from getBoundingClientRect() which always gives the real viewport-relative coordinates regardless of how the style was written. Bug 2 — Position NaN out of range on undo: ProseMirror can destroy and remount a NodeView on undo. When QuestionView is remounted it found an existing questionMap entry and skipped registration entirely, leaving a stale getPos closure in the map. dispatchInner then called the old getPos (now invalid), got undefined, and passed StepMap.offset(NaN) to the outer transaction, triggering ProseMirror's range error. Fix: always refresh node and getPos in the map entry on remount. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Guard dispatchInner: skip outer dispatch if getPos() returns undefined (node no longer in document) and skip null-mapped steps, preventing the RangeError: Position NaN out of range that fired on every keystroke and undo/redo cycle in the popup editor. - Guard update(): null-check findDiffEnd result before destructuring to avoid undefined endA/endB being passed into tr.replace(). - Add "Mod-Shift-z": redo to the popup editor keymap so Cmd+Shift+Z works for redo alongside the existing Cmd+Y binding. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After closePopup() removed the DOM element, the browser moved focus to document.body, making Cmd+Z unreachable on the outer editor. In practice this meant deleting a question via the trash button was not undoable. Add view.focus() at the end of closePopup() so focus always returns to the outer ProseMirror editor, regardless of which close path is taken (trash button, × button, Escape, or touch). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
hendriebeats
added
Size: Large
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracts the question feature out of the monolithic
Editor.tsxinto a dedicatedQuestionPlugin.ts, and ships several fixes and improvements made during that work.What changed
Module extraction
Editor.tsxintofrontend/src/Editor/QuestionPlugin.tsand re-exported;Editor.tsxnow imports from it.Resizable popup
Verse context popover
Bug fixes
getPoson undo:QuestionViewnow refreshesquestionMap[id].getPoson every remount (e.g. after undo), sodispatchInneralways calls the live position callback.position: absolute(page-relative) toposition: fixed(viewport-relative), and fromdocument.body.scrollHeighttowindow.innerHeight, since only#mainEditorHolderscrolls — not the page itself.window.removeEventListener("resize", clampToViewport)is now called when the popup closes.dispatchInnernow skips steps wherestep.map(offsetMap)returns null, preventing a potential crash.getPosundefined guard:dispatchInnerearly-returns ifqNode.getPos()is undefined (can happen transiently during undo).findDiffEndnull guard:QuestionView.updatenow checks for a null return fromfindDiffEndbefore destructuring.min-widthraised from 200px to 250px to match the 250px floor enforced in the JS resize handler.Mod-Shift-Zredo: added to the inner editor's keymap (was missing;Mod-Ywas the only redo binding).view.focus()is called when the popup closes, returning keyboard focus to the outer editor.Accessibility / polish
<div>to<button>witharia-label; keyboard-activatable via Enter/Space.<closer>custom element to<button>witharia-label.noQuestionsTextbuilt with DOM API instead ofinnerHTMLto avoid XSS viawindow.base.cursor: default; user-select: noneso it doesn't look like editable text.