Conversation
ebf9d1b to
d4b023b
Compare
There was a problem hiding this comment.
Pull request overview
This PR parameterizes page turn behavior and adopts a hyperlink convention (e.g., {Example https://example.com}), making a breaking change to the Document constructor that requires a major version bump.
- Refactors Document constructor to accept an options object instead of positional parameters
- Implements automatic hyperlink detection and creation for URLs at the end of text
- Adds configurable page turn behavior with 'log', 'remove', and 'fade' options
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| entry.js | Updates Document instantiation to use new options-based constructor with pageTurnBehavior set to 'fade' |
| engine.js | Moves display() call before handler.end() to ensure proper display timing at story end |
| document.js | Refactors constructor to accept options object, implements hyperlink convention with regex matching, and parameterizes page turn behavior in clear() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO merge with prior text node | ||
| this.cursor.appendChild(document.createTextNode(lift + text)); | ||
| } else { | ||
| // Support a hyperlink convention. | ||
| if (lift !== '') { | ||
| this.cursor.appendChild(document.createTextNode(lift)); | ||
| } | ||
| const link = document.createElement('a'); | ||
| link.href = match[1]; | ||
| link.target = '_blank'; | ||
| link.rel = 'noreferrer'; | ||
| link.appendChild(document.createTextNode(text.slice(0, match.index))); | ||
| this.cursor.appendChild(link); |
There was a problem hiding this comment.
Inconsistent indentation: this block uses 4 spaces while the rest of the file appears to use 2 spaces. Maintain consistent indentation throughout the file.
| // TODO merge with prior text node | |
| this.cursor.appendChild(document.createTextNode(lift + text)); | |
| } else { | |
| // Support a hyperlink convention. | |
| if (lift !== '') { | |
| this.cursor.appendChild(document.createTextNode(lift)); | |
| } | |
| const link = document.createElement('a'); | |
| link.href = match[1]; | |
| link.target = '_blank'; | |
| link.rel = 'noreferrer'; | |
| link.appendChild(document.createTextNode(text.slice(0, match.index))); | |
| this.cursor.appendChild(link); | |
| // TODO merge with prior text node | |
| this.cursor.appendChild(document.createTextNode(lift + text)); | |
| } else { | |
| // Support a hyperlink convention. | |
| if (lift !== '') { | |
| this.cursor.appendChild(document.createTextNode(lift)); | |
| } | |
| const link = document.createElement('a'); | |
| link.href = match[1]; | |
| link.target = '_blank'; | |
| link.rel = 'noreferrer'; | |
| link.appendChild(document.createTextNode(text.slice(0, match.index))); | |
| this.cursor.appendChild(link); |
| if (lift !== '') { | ||
| this.cursor.appendChild(document.createTextNode(lift)); | ||
| } | ||
| const link = document.createElement('a'); |
There was a problem hiding this comment.
Use 'const' or 'let' instead of 'var' for the link variable declaration. Using 'var' is inconsistent with modern JavaScript best practices and can lead to unexpected scoping behavior.
| this.frame.style.transform = 'translateX(-2ex)'; | ||
| this.frame.addEventListener('transitionend', this); | ||
| if (this.pageTurnBehavior === 'log') { | ||
| this.options.remove(); |
There was a problem hiding this comment.
The case 'log' appears to reference 'this.options.remove()' but should reference 'this.frame.remove()'. The 'log' behavior seems intended to keep previous pages in the document (for logging), but currently it only removes the options element, not handling the entire frame appropriately. This likely causes a bug where frames accumulate incorrectly.
| this.options.remove(); | |
| this.frame.remove(); |
| link.href = match[1]; | ||
| link.target = '_blank'; | ||
| link.rel = 'noreferrer'; | ||
| link.appendChild(document.createTextNode(text.slice(0, match.index))); |
There was a problem hiding this comment.
The hyperlink implementation does not handle the case where the link text itself is empty (when match.index equals text.length). This would create an anchor element with no visible text, which is confusing for users and bad for accessibility.
| link.appendChild(document.createTextNode(text.slice(0, match.index))); | |
| const linkText = text.slice(0, match.index); | |
| link.appendChild(document.createTextNode(linkText !== '' ? linkText : match[1])); |
| const link = document.createElement('a'); | ||
| link.href = match[1]; | ||
| link.target = '_blank'; | ||
| link.rel = 'noreferrer'; |
There was a problem hiding this comment.
Missing 'noopener' in the rel attribute. When using target='_blank', it's a security best practice to include both 'noopener' and 'noreferrer' to prevent the new page from accessing the window.opener property and protect against potential security vulnerabilities.
| link.rel = 'noreferrer'; | |
| link.rel = 'noreferrer noopener'; |
{Example https://example.com}endhook, so the hook and react to the new frame displayed.