Conversation
There was a problem hiding this comment.
great work so far, please review the comments and implement changes requested.
Testing is also a very important factor to consider.This new interactive annotation feature is a critical part of the application. To ensure its stability and prevent future regressions, it needs to be covered by automated tests.
Please add a new test suite to templates/test.js. These tests should verify the following:
1.When displayInteractiveAnnotations is called, the correct number of .annotation-marker elements are created in the DOM.
2.Simulating a "mouseenter" event on a marker successfully displays its tooltip.
3.Simulating a "mouseleave" event on a marker successfully hides the tooltip.
4.Also ensure testing covers cross-browser compatibility (especially mobile browsers), API failure scenarios (network errors, empty responses),Performance with many annotations and Coordinate edge cases (coordinates outside expected ranges).
| COMBINED_DATA: "/combined-data", | ||
| BONESET_DATA: "/api/boneset", | ||
| MOCK_BONE_DATA: "./js/mock-bone-data.json" | ||
| } |
There was a problem hiding this comment.
I see 3 endpoints for this file, this is very confusing , let restructure this and have a single source for a unified service
There was a problem hiding this comment.
I've addressed this by:
Removed the COMBINED_DATA and MOCK_BONE_DATA endpoints - now we only use /api/boneset
Removed the fetchMockBoneData() function entirely
Added extractDropdownData() helper function to transform boneset data into dropdown format
Removed the default parameter from fetchBonesetData() so the boneset ID must be explicitly passed
The API now has a single unified data source as requested.
templates/js/api.js
Outdated
| } | ||
| } | ||
|
|
||
| // Keep the mock data function as fallback |
There was a problem hiding this comment.
The file now contains three separate functions for fetching data (fetchCombinedData, fetchBonesetData, fetchMockBoneData). this will become confusing in the future for us, lets consider consolidating these into a more unified service.
There was a problem hiding this comment.
Removed fetchCombinedData() and fetchMockBoneData() functions entirely
Now only use fetchBonesetData() as the single data-fetching function
Added extractDropdownData() helper to transform the boneset data into the format needed for dropdowns
Updated main.js to call fetchBonesetData() once and then use extractDropdownData() to get dropdown data from it
The application now has one clear data flow: fetch boneset → extract what's needed for UI, instead of multiple confusing endpoints.
| export async function fetchBonesetData(bonesetId = "bony_pelvis") { | ||
| const API_URL = `${API_CONFIG.BASE_URL}${API_CONFIG.ENDPOINTS.BONESET_DATA}/${bonesetId}`; | ||
|
|
||
| try { |
There was a problem hiding this comment.
The function correctly throws an error on a failed request. However, the responsibility for handling this error is now entirely on the calling function (main.js). We need to ensure that the UI gracefully handles a failure to load this critical data.
There was a problem hiding this comment.
Implemented user-facing error handling. Added displayErrorMessage() function in main.js that shows a styled error message in the UI when the API fails, instead of just console logging. The error message tells users to check if the server is running and suggests refreshing.
templates/js/main.js
Outdated
|
|
||
| let combinedData = { bonesets: [], bones: [], subbones: [] }; | ||
| let mockBoneData = null; | ||
| let liveBonesData = null; // NEW: Live data from API |
There was a problem hiding this comment.
The code now manages several different data variables (combinedData, mockBoneData, liveBonesData). Although this works, it adds a layer of complexity. we should aim to have a single, consistent data structure that the application works with after the initial load. A refactoring is needed here.
There was a problem hiding this comment.
Refactored to use single appData object with two properties:
appData.boneset - raw API data
appData.dropdown - transformed dropdown data
Removed the separate combinedData, mockBoneData, and liveBonesData variables. Everything now uses this one data structure throughout the application
templates/js/viewer.js
Outdated
| marker.style.height = `${Math.max(scaledHeight, 20)}px`; | ||
|
|
||
| // DEBUG: Add visible styling to make sure markers are visible | ||
| marker.style.backgroundColor = "red"; |
There was a problem hiding this comment.
There is some debug styling left in the JavaScript (e.g., marker.style.backgroundColor = "red"). All styling should be handled in the CSS file.
There was a problem hiding this comment.
Removed all debug styling (backgroundColor, border, zIndex) and debug console.log statements from viewer.js. Markers now use only the CSS styles. Kept the essential positioning styles (position, left, top, width, height) which are required for functionality
templates/style.css
Outdated
|
|
||
| .annotation-marker { | ||
| animation: markerAppear 0.3s ease-out; | ||
| } No newline at end of file |
There was a problem hiding this comment.
The CSS is well-written, well-organized, and follows good practices.
templates/js/viewer.js
Outdated
| annotationsOverlay.appendChild(annotationsList); | ||
| // Assume the original coordinate system is approximately 12000000 x 8000000 (common PowerPoint dimensions) | ||
| const ORIGINAL_WIDTH = 12000000; | ||
| const ORIGINAL_HEIGHT = 8000000; |
There was a problem hiding this comment.
i see an assumption of "12M x 8M coordinate system" without clear documentation of how this was determined. This hard-coded assumption could break if the API returns different coordinate ranges. lets improve this by adding validation or dynamic coordinate range detection.
There was a problem hiding this comment.
Replaced hard-coded 12M x 8M assumption with dynamic coordinate detection. Added detectCoordinateRange() function that analyzes actual annotation positions to determine the coordinate system bounds. This makes the code adapt to whatever coordinate system the API uses instead of assuming PowerPoint dimensions.
templates/js/api.js
Outdated
| } | ||
|
|
||
| // NEW: Fetch live boneset data from the API | ||
| export async function fetchBonesetData(bonesetId = "bony_pelvis") { |
There was a problem hiding this comment.
this currently only connects to (/api/boneset/bony_pelvis) endpoint
which limits the application to only pelvic bones. to remove limitation in our application, lets make the endpoint configurable or dynamic based on bone selection.
2 participants
Description
Fixes #128
Please include a summary of the changes and the related issue. Include any relevant motivation and context. List any dependencies that are required for this change.
A pull request must contain the following elements.
1. What was Changed
The application was transformed from using mock data to a live API-driven system with interactive annotation markers. Users can now select bones and see clickable markers positioned on bone images that display annotation text in tooltips when hovered over or clicked.
2. Why it was Changed
This change was required to meet the core objectives of issue_128 and advance the project from Sprint 1's static implementation. The mock data system was a temporary placeholder that needed to be replaced with real API integration. Interactive markers were essential to provide users with an engaging, educational experience where they can explore anatomical features directly on bone images rather than just reading static lists.
3. How it was Changed
The implementation involved four main technical changes:
API Integration: Modified api.js to add fetchBonesetData() function that connects to the /api/boneset/bony_pelvis endpoint, replacing the mock data system.
Data Flow Update: Updated main.js to use live API data through the new findBoneById() helper function that searches the nested bone/subbone structure returned by the API.
Interactive Annotation System: Completely rewrote the displayAnnotations() function in viewer.js to create positioned DOM elements. The main challenge was coordinate scaling - the API returns coordinates in a high-resolution system (millions of pixels) that needed to be normalized to fit display dimensions. This was solved by assuming a 12M x 8M coordinate system and scaling down proportionally.
Styling and UX: Added CSS for semi-transparent blue circular markers with hover effects, tooltips with smart positioning, and responsive design considerations.
Technical Challenge: The biggest challenge was coordinate transformation. Initial attempts resulted in markers positioned millions of pixels off-screen. The solution involved recognizing that the coordinates came from a PowerPoint-like coordinate system and implementing proper normalization scaling.
4. Screenshots (if applicable)