Conversation
|
can you resolve the issue with the failed lint check so it passes our workflow setup |
templates/js/main.js
Outdated
| let mockBoneData = null; | ||
|
|
||
| // Mock data fetching function | ||
| async function fetchMockBoneData() { |
There was a problem hiding this comment.
I think you did an excellent job of separating the functions and making it clear as to what each function does, and also including error checkers in some of the functions. As someone who is still getting used to the language, you made it very easy to understand!
templates/js/main.js
Outdated
| } | ||
|
|
||
| // Update the image | ||
| const boneImage = document.getElementById('bone-image'); |
There was a problem hiding this comment.
I think it might be benefitcal to add error handling for this image, as the user might be confused if it doesn't load and it's just white space.
UcheWendy
left a comment
UcheWendy
left a comment
There was a problem hiding this comment.
Thank you for this pull request. Getting the images and annotation text to display is a huge step forward and brings the core of the viewer to life. The feature works, which is the most important part. My feedback which i have wriiten as inline comments are focused on making the code more robust, readable, and ready for the switch to a live API in the next sprint.
This new functionality is central to our application, so it's critical that we add tests for it.
Please add a new test suite to templates/test.js for the viewer's display logic. You can use the mock data file to write tests that verify:
-
After a selection, the src attribute of the bone-image element is correctly updated.
-
The correct number of annotation text elements are created inside the #annotations-overlay div.
-
When a bone with no image is selected, the src is set to a placeholder path.
| ] | ||
| } | ||
| ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
The mock data file is well-structured and accurately mimics the expected format of a complete bone object. This is exactly what was needed for the frontend to be developed independently.
However, The mock file should include examples for a few different scenarios to help with testing. For instance, it should include an object for a bone that has no annotations and another for a bone that is missing an image_url. This will help us ensure the frontend handles these cases gracefully.
Please expand the mock data to include these edge cases so we can verify the frontend doesn't break when the data isn't perfect.
templates/js/main.js
Outdated
| dropdown.appendChild(option); | ||
| }); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The core logic is in place. The code successfully fetches the mock data and uses it to update the viewer when a dropdown selection changes. The event listeners in dropdowns.js are correctly set up to trigger the new display logic.
1)My Concern lies in the fact that the logic for updating the UI (changing the image source, rendering annotation text) is mixed in with the data-fetching and event-handling code. To make this more maintainable, we should separate these concerns. I recommend creating a new, dedicated module for managing the viewer's state.
-
Another concern is what happens if an image fails to load (e.g., due to a broken URL in the data)? The application should handle this gracefully instead of showing a broken image icon.
-
The path to the mock data file is likely hardcoded directly in the fetch call. While okay for this sprint, we should centralize all our API paths and URLs into a single configuration object or file (e.g., in api.js) to make it easier to switch from mock data to the live API later.
Consider creating a new file, templates/js/viewer.js, with functions like displayBoneImage(boneData) and displayAnnotations(boneData). Your main.js can then call these functions, keeping the logic clean and organized.
UcheWendy
left a comment
UcheWendy
left a comment
There was a problem hiding this comment.
Good work, all acceptance criterias has being meet. You have also addressed all the requirements of the issue and the feedback from the previous review. The code is clean, robust, and well-tested. Excellent work.
…display-using-mock-data
| expect(item.className).toBe("annotation-item"); | ||
| }); | ||
| ======= | ||
| }); |
There was a problem hiding this comment.
delete this === as it should not be in a javascript code file
UcheWendy
deleted the
120-frontend-logic-for-image-and-annotation-display-using-mock-data
branch
3 participants
Description
Fixes #120 (A reference to the issue that the PR resolves. For example, if a PR resolves issue #47, the first comment in the PR should be: Fixes #47 (GitHub will then automatically link your PR to the issue it resolves.))
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
Added mock data display functionality that shows a placeholder bone image and a formatted list of anatomical annotations when a user selects a bone from the dropdown menu. The interface now provides visual feedback and educational content for bone selection.
2. Why it was Changed
This change was needed to enable concurrent development between frontend and backend teams during Sprint 1. Without this mock data implementation, the frontend team would be blocked waiting for the backend API to be completed. The mock data allows for immediate testing and development of the user interface while maintaining the same data structure that will be used with the live API.
3. How it was Changed
Created templates/js/mock-bone-data.json containing sample bone data with the required structure (id, name, image_url, annotations array)
Modified templates/js/main.js to include:
fetchMockBoneData() function to load the local JSON file
displayBoneData() function to update the image and annotations when a bone is selected
displayAnnotations() function to create a formatted list of anatomical features
Event listener integration with existing bone dropdown selection logic
Updated templates/style.css with responsive styling for the image/annotations layout:
Flexbox layout for side-by-side image and annotations display
Styled annotation items with blue accent borders and proper spacing
Responsive design for mobile devices
The implementation uses the existing dropdown selection mechanism and integrates seamlessly with the current codebase. When the live API is ready, only the displayBoneData() function needs to be modified to fetch from the API endpoint instead of the local JSON file.
4. Screenshots (if applicable)
Add the before and after screenshots for UI related changes
