Works on Issue 249 which gives functionality to the next and prev button - #409
Conversation
Remex9
left a comment
There was a problem hiding this comment.
Summary
Your fix overall looks good.
The #248 fix makes sense. Clearing bone/sub-bone used to just blank everything with showPlaceholder(), and now it falls back to whatever parent is still selected. That’s what the issue was asking for.
Also noticed you fixed #249 in here too (Prev/Next sync). Using .value + dispatching change instead of selectedIndex + only updating the description is the right call, explains why the description was updating but the dropdown/image weren’t.
Tests look good and CI is green. Ran the new ones locally and they passed.
Suggestions
PR title / description : Title is still “Mayors branch,” and the body only says Closes #248. Since this also fixes #249, you could update the title and add Closes #249 so that issue actually closes.
First Next skips the first sub-bone :After selecting a bone, index starts at 0 while the dropdown is still on the placeholder, so the first Next jumps to the second sub-bone. Your test currently expects that (subbone_b after one Next). Not a blocker for this PR, but worth a follow-up.
There was a problem hiding this comment.
Thanks for the updates. The fallback behavior for deselecting a bone or sub-bone looks good, and the added tests are helpful. Before approval, please address the following:
- Update the PR title from “Mayors branch” to clearly describe the changes.
- Update the PR description to explain both Issue #248 and Issue #249, and add Closes #249 if this PR is intended to close it.
- Verify the Next button behavior. It currently appears to skip the first sub-bone after a bone is selected. Please fix this if unintended, or explain why the current behavior is expected.
- Add or update a test confirming that the correct parent images are displayed after deselecting a bone or sub-bone.
- Confirm that all tests, lint checks, and CI checks pass after the updates.
- Request another review from the code owners once these items are complete.
The main Issue #248 fix appears to be on the right track. Once the checklist is addressed, we can review the updated changes again.
|
This PR fixes two related bugs in the bone viewer's dropdown/navigation logic: #248: Deselecting a bone or sub-bone used to blank the entire display instead of falling back to the parent boneset's or bone's information. Issue #248 — templates/js/dropdowns.js The change listeners for the bone and sub-bone elements only handled the "something was selected" case. When cleared back to the default option, both listeners called showPlaceholder() directly, wiping the display instead of checking whether a parent level (boneset, or bone) was still selected. Fixed by checking the parent dropdown's value first and falling back to its description/images; the placeholder now only shows when there's no parent selection either. Issue #249 — templates/js/navigation.js Three related bugs in the Prev/Next navigation: updateUI() set subboneDropdown.selectedIndex directly and called the description updater as a plain function call. Setting .selectedIndex programmatically does not fire a native "change" event, so the listeners that load images (dropdowns.js) and the HTMX-driven description fetch never ran — only the description updated, via a redundant direct call. Fixed by setting .value and dispatching a real "change" event, so Prev/Next now goes through the same path as a manual dropdown selection. This also removed the now-dead updateDescription parameter from setupNavigation (and its caller in main.js), per the redundancy noted in the issue comments. That same .selectedIndex assignment was also off by one: the real dropdown always has a placeholder option before the subbone options, so the index into the subbones array didn't match the dropdown's actual option positions. Fixed by matching on .value instead of position. Found during review: clicking Next for the first time after selecting a bone skipped the first sub-bone and jumped to the second, because currentSubboneIndex was initialized to 0 (treating the first sub-bone as "already shown") while the dropdown itself still displayed its placeholder. Fixed by initializing to -1 ("nothing shown yet"), so the first Next click correctly reveals subbones[0]. Testing: templates/tests/dropdowns.test.js and templates/tests/navigation.test.js cover both fixes, including explicit assertions that the correct parent's images (not just its description) are fetched and rendered after a deselect, and a dedicated regression test for the first-subbone-skip bug. All were verified to actually catch their respective bugs by temporarily reverting each fix and confirming the tests failed as expected. |
c617a4f to
f9645e5
Compare
mudabs
left a comment
There was a problem hiding this comment.
Thank you! The branch name is now consistent with the changes made
Pull Request Summary
Closes #248
This feature enables that when a user chooses a bon or a bone part but deselects it, it doesn't default to a plain screen, rather it shows information on the parent bone or boneset
Screenshots
PR Checklist
Detailed Description
In templates/js/dropdowns.js, the change listeners for the bone and sub-bone elements only handled the "something was selected" case. When the selection was cleared back to the default option, both listeners fell straight to calling showPlaceholder(), wiping the display instead of checking whether a parent level (boneset, or bone) was still selected. Fix: In both the bone-change and sub-bone-change listeners, the else branch (fired when the dropdown is reset) now checks the parent dropdown's current value first: Deselecting a bone now checks bonesetSelect.value — if a boneset is still selected, it reloads that boneset's description and images instead of blanking the screen. Deselecting a sub-bone now checks boneSelect.value the same way, falling back to the parent bone's info. The placeholder is only shown if there's no parent selection either (e.g. the boneset was cleared too). Added templates/tests/dropdowns.test.js with three Jest tests covering: deselecting a bone falls back to boneset info, deselecting a sub-bone falls back to bone info, and deselecting with no parent selected still shows the placeholder. I verified these tests actually catch the regression by temporarily reverting the fix and confirming two of the three failed as expected. Also manually walked through the reported repro steps in the browser to confirm the fix visually.