all comments added#360
Conversation
There was a problem hiding this comment.
Pull request overview
Adds/updates JSDoc-compliant documentation across templates/js to improve developer onboarding and make function/class intent, parameters, and return types clearer (per #213).
Changes:
- Added/expanded JSDoc blocks for key exported functions and several internal helpers (including
@param/@returns). - Improved inline documentation for UI modules (sidebar, search, navigation, quiz).
- Added type documentation for shared data structures (e.g.,
imageCaptionsmap).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/js/viewer.js | Adds missing @returns and improves parameter doc for viewer display helpers. |
| templates/js/sidebar.js | Adds JSDoc for async sidebar/help-button initialization functions. |
| templates/js/search.js | Adds JSDoc for search initialization and internal search/navigation helpers. |
| templates/js/quiz.js | Adds class/method JSDoc for quiz manager behavior and return types. |
| templates/js/navigation.js | Documents navigation setup and helper functions with parameter/return types. |
| templates/js/main.js | Adds JSDoc for populateSubboneDropdown (but currently has a malformed/duplicated block). |
| templates/js/imageDisplay.js | Adds JSDoc for image rendering helpers and transform utilities. |
| templates/js/imageCaptions.js | Documents exported caption lookup map type/shape. |
| templates/js/dropdowns.js | Adds JSDoc for image stage helpers, image-loading helper, and dropdown wiring. |
| templates/js/description.js | Adds JSDoc for GitHub-backed description loader. |
| templates/js/coloredRegionsOverlay.js | Adds missing @returns tags for clear helpers. |
| templates/js/api.js | Adds JSDoc for API fetch functions including throws/return details. |
| templates/js/annotationOverlay.js | Adds JSDoc for overlay lifecycle and annotation rendering/loading helpers (one doc mismatch with implementation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Populates the subbone `<select>` element with options for the given subbone IDs. | ||
| * Inserts a placeholder option and disables the dropdown if no subbones are provided. |
| * Reads normalized geometry and slide dimensions from the JSON to map PowerPoint | ||
| * EMU coordinates onto the displayed pixel size of the container. | ||
| * @param {HTMLElement} container - The element to draw annotations into. | ||
| * @param {Object} annotationsJson - Annotation data from the API, containing: | ||
| * `annotations` {Array}, `normalized_geometry` {Object}, `slide_width` {number}, | ||
| * and `slide_height` {number}. |
There was a problem hiding this comment.
Correct—this function expects the fields normalized_geometry and either annotations or text_annotations. Although really I think that's a bit messy...why is this function expecting two possible formats, when really only one should exist? The line where it's checking for either annotations or text_annotations should be modified such that it's only looking for one of the two—whichever one actually is expected to exist.
There was a problem hiding this comment.
Also: The expected properties should be listed in the format documented here: https://jsdoc.app/tags-param#parameters-with-properties
You've already done this correctly with imageDisplay.applyRotation
| /** | ||
| * Clear all colored region overlays from a container | ||
| * @param {HTMLElement} container - The container element | ||
| * @returns {void} |
There was a problem hiding this comment.
For what it's worth, adding @returns {void} isn't really necessary, but it's fine that's it's here. No action needs to be taken on this.
| const GITHUB_RAW_URL = "https://raw.githubusercontent.com/oss-slu/DigitalBonesBox/data/data/descriptions/"; | ||
|
|
||
| /** | ||
| * Fetches the description JSON for the given bone/subbone ID from GitHub and |
There was a problem hiding this comment.
Since we're transitioning away from fetching our data from GitHub, this should no longer explicitly say that the data is coming from there. In fact, it should not specify where the data is coming from at all, since we're planning on refactoring this to go through other functions anyway.
| /** Helper: fetch images for a bone/sub-bone and render them | ||
| * @param {string} boneId - The bone or subbone ID to load images for. | ||
| * @param {Object} [options={}] - Options forwarded to `displayBoneImages`, e.g. | ||
| * `{ annotationsUrl: string, boneId: string, isBonesetSelection: boolean }`. |
There was a problem hiding this comment.
Don't include the example of the options here. If the options that displayBoneImages expects are changed, it would be really easy to miss updating this bit of documentation here.
| /** ---- Empty-state / clearing ------------------------------------------- */ | ||
| /** | ||
| * Renders the empty-state placeholder message inside the image container | ||
| * and clears all annotations, colored regions, and captions. |
There was a problem hiding this comment.
Clarify that "annotations" refers to text annotations specifically. (In some parts of the repo, we also use "annotation" to refer to the colored region annotations, or other image modifications entirely, so we want to be specific when referring to text annotations.)
| } | ||
|
|
||
| /** | ||
| * Clears all images, annotations, colored regions, and captions from the image container. |
There was a problem hiding this comment.
Same as above: "annotations" → "text annotations"
| * Applies a CSS rotation (and optional horizontal flip) to an image element | ||
| * based on the PowerPoint rotation template for the given view. | ||
| * @param {HTMLImageElement} imgEl - The image element to transform. | ||
| * @param {Object} [options={}] - Rotation parameters. |
There was a problem hiding this comment.
This incorrectly lists the default value for options
| } | ||
|
|
||
| /** | ||
| * Decrements the current subbone index (moves to the previous subbone), if possible. |
There was a problem hiding this comment.
"if possible" should be clarified to "if greater than 0"
| } | ||
|
|
||
| /** | ||
| * Increments the current subbone index (moves to the next subbone), if possible. |
There was a problem hiding this comment.
Same as above--clarify conditions for "if possible"
|
|
||
| /** | ||
| * Generate wrong answer choices from the master pool | ||
| * Picks `count` distractor names from the master pool, excluding the |
There was a problem hiding this comment.
I think this sounds a bit unclear, and that the original comment made the function's overall purpose a bit more understandable by saying "generate wrong answer choices." I just don't like the term "distractor name," so maybe this could be reworded to clarify a bit by clarifying that this is for selecting wrong answer choices to display to the user.
| /** | ||
| * Display answer choice buttons | ||
| * Renders the answer choice buttons for the given question. | ||
| * @param {{allAnswers: string[], correctAnswer: string}} question - The |
There was a problem hiding this comment.
The structure of question would be better documented as outlined here: https://jsdoc.app/tags-param#parameters-with-properties. This would ensure that IDEs and other tools would parse this documentation correctly.
Closes #213
Add JSDoc-compliant comments to files that lack sufficient documentation.