Hide annotations in speedgrader #2227
Conversation
📝 WalkthroughWalkthroughThe pull request introduces new JavaScript functions and HTML elements to enhance the visibility control of annotations within the application. Specifically, it adds functions to show and hide annotations and integrates these functionalities into the user interface through new buttons. The changes ensure that annotations can be toggled based on user interactions, improving the overall user experience for instructors and course assistants. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/views/submissions/view.html.erb (1)
149-150: LGTM! Consider reducing code duplication.The addition of "Hide Comments" and "Show Comments" buttons to the student view is consistent with the PR objective of improving readability for all users. This implementation ensures that both instructors and students can benefit from the new feature.
To reduce code duplication, consider extracting the button HTML into a partial view that can be rendered in both the instructor and student sections. This would make future maintenance easier. Here's an example of how you could refactor this:
- Create a new partial file, e.g.,
_annotation_toggle_buttons.html.erb:<button id="hideAnnotations" class="btn small" onclick="hideAnnotations()">Hide Comments</button> <button id="showAnnotations" style="display: none;" class="btn small" onclick="showAnnotations()">Show Comments</button>
- Replace the button code in both sections with a render call:
<%= render partial: 'annotation_toggle_buttons' %>This refactoring is optional but could improve code maintainability.
app/assets/javascripts/annotations.js (3)
1580-1585: LGTM: Effective implementation of hide annotations featureThe
hideAnnotations()function correctly implements the feature to hide annotations. It properly toggles button visibility, removes annotation lines, and refreshes the UI.Consider adding a CSS class to toggle visibility instead of directly manipulating style properties. This would improve separation of concerns:
- document.getElementById("hideAnnotations").style.display = "none"; - document.getElementById("showAnnotations").style.display = "inline-flex"; + document.getElementById("hideAnnotations").classList.add("hidden"); + document.getElementById("showAnnotations").classList.remove("hidden");Then define the
hiddenclass in your CSS:.hidden { display: none !important; }
1587-1592: LGTM: Well-implemented show annotations featureThe
showAnnotations()function effectively implements the feature to show annotations. It correctly toggles button visibility, displays annotations, and refreshes the UI.As suggested for
hideAnnotations(), consider using CSS classes for toggling visibility:- document.getElementById("showAnnotations").style.display = "none"; - document.getElementById("hideAnnotations").style.display = "inline-flex"; + document.getElementById("showAnnotations").classList.add("hidden"); + document.getElementById("hideAnnotations").classList.remove("hidden");
1345-1348: Overall implementation of hide/show annotations feature is solidThe new hide/show annotations feature is well-implemented with clear, distinct functions for each action (
commentsVisible(),hideAnnotations(), andshowAnnotations()). The integration with existing code, such as insubmitNewAnnotation(), is done correctly.For consistency and improved maintainability, consider extracting the button IDs into constants at the top of the file:
const HIDE_ANNOTATIONS_BUTTON_ID = "hideAnnotations"; const SHOW_ANNOTATIONS_BUTTON_ID = "showAnnotations";Then use these constants throughout the code. This would make it easier to update button IDs if needed and reduce the risk of typos.
Also applies to: 1574-1592
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/assets/javascripts/annotations.js (2 hunks)
- app/views/submissions/view.html.erb (2 hunks)
🔇 Additional comments (4)
app/views/submissions/view.html.erb (2)
132-133: Great implementation! Fully addresses PR objectives.The changes successfully implement the feature to hide annotations in the speedgrader, as requested in issue #2203. By adding toggle buttons for both instructors/course assistants and students, the implementation improves the user experience and enhances the readability of student submissions.
The changes are minimal, focused, and consistent, which reduces the risk of unintended side effects. Overall, this implementation effectively addresses the PR objectives and provides a valuable enhancement to the Autolab application.
Also applies to: 149-150
132-133: LGTM! Verify JavaScript functions.The addition of "Hide Comments" and "Show Comments" buttons aligns well with the PR objective of allowing users to hide annotations in the speedgrader. The implementation is consistent with the existing UI.
Please ensure that the
hideAnnotations()andshowAnnotations()JavaScript functions are properly defined and functional. Run the following script to verify:✅ Verification successful
JavaScript functions are present and correctly defined.
The
hideAnnotations()andshowAnnotations()functions exist inapp/assets/javascripts/annotations.jsand are correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of hideAnnotations() and showAnnotations() functions # Test: Search for the function definitions rg --type js 'function (hide|show)Annotations\(\)'Length of output: 186
app/assets/javascripts/annotations.js (2)
1345-1348: LGTM: New visibility check for annotation boxesThe added condition
if (!commentsVisible())ensures that new annotation boxes are only appended when comments are visible. This change aligns well with the new feature to hide/show annotations and is implemented correctly.
1574-1578: LGTM: Well-implemented visibility check functionThe
commentsVisible()function is a clean and effective way to determine if comments are currently visible. It usesgetComputedStyleto reliably check the visibility of the "hideAnnotations" button, which is a good approach.
jlge
requested review from
a team and
20wildmanj
and removed request for
a team
|
When you hide comments, you can still navigate to them using the annotations tab, but they aren't visible. Any thoughts on if they should become visible when you navigate to them? Also, you can add annotations while annotations are hidden, and then the annotations are still hidden, not sure if that's the best user experience either. |
Yeah I was thinking about what the behavior should be and I felt like consistent behavior made the most sense for me (all hidden or all shown) since it could get confusing, but having the ability to hide on a more granular level could be helpful too idk tbh,, I will ask around maybe |
Description
Motivation and Context
Resolves #2203
How Has This Been Tested?
As instructor
As student
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.