[Refactor:Forum] Replace popups with tabs#12678
Conversation
Partially addresses Submitty#3271 Remove window.open() popup parameters (toolbar, scrollbars, width, height) to allow browsers to open URLs in standard new tabs instead of restricted popup windows. This provides a better user experience as users can freely resize, bookmark, and navigate these pages like normal browser tabs. Changes: - server.js: Simplified downloadStudentAnnotations(), openUrl(), and openSubmittedFile() to use window.open(url, '_blank') - forum.js: Updated openFileForum(), openInWindow(), and attachment image click handlers - drag-and-drop.js: Updated openFile() - stat-page.js: Added explicit '_blank' target for consistency Note: openPopUp() and openActionsPopup() functions that write HTML content to popup windows are left unchanged as they require a different refactoring approach.
Asterisk-Hunter
changed the title
|
Tested locally and attached a video. Forum stats thread links now open in a normal tab for me, but the grading files path still opens a separate browser window:
Expected a normal browser tab; got a separate browser window on my system. I also still see stale “pop up” wording in the grading UI, so I don’t think this is fully resolved yet. testing_pr.webm |
|
Thanks for the detailed test ,the video made it easy to pinpoint what was still wrong. Two things were off:
The |
Rkoester47
left a comment
Rkoester47
left a comment
There was a problem hiding this comment.
I tested the changes in this PR and can confirm that all of the areas described in the testing steps do pop out as a new tab now. I am a fan of tabs over a new window, especially a small window. As far as I can tell, this PR does what it sets out to do without any extra changes. Good work
github-project-automation
Bot
moved this from Seeking Reviewer
to Awaiting Maintainer Review
in Submitty Development
|
Thanks for the approval! Happy to finally get this merged. Let me know if you have any other issues I can help with. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12678 +/- ##
=========================================
Coverage 21.62% 21.62%
Complexity 9824 9824
=========================================
Files 268 268
Lines 36816 36816
Branches 490 490
=========================================
Hits 7960 7960
Misses 28370 28370
Partials 486 486
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I noticed the leaderboard test is timing out this looks like a pre-existing flaky test issue (not related to my changes). The test is about tied rankings on the leaderboard, which is completely separate from the popup-to-tab changes. Let me know if there's anything I need to do on my end, otherwise hoping this can be merged once CI passes. |
Rkoester47
left a comment
Rkoester47
left a comment
There was a problem hiding this comment.
I am reapproving this PR, since the functionality works. However, for some reason the Cypress Feature test is failing on Github. However, it passed locally on many reruns, and this might need to be addressed before merging.
bmcutler
moved this from Awaiting Maintainer Review
to Work in Progress
in Submitty Development
|
@Asterisk-Hunter @dandrecollins07-ctrl |
github-project-automation
Bot
moved this from Work in Progress
to Done
in Submitty Development
dandrecollins07-ctrl
left a comment
dandrecollins07-ctrl
left a comment
There was a problem hiding this comment.
Passes various test cases, JavaScript lint case fixed. Approving.
5 participants
Why is this Change Important & Necessary?
Partially addresses #3271
Right now several places in the codebase use
window.open()with popup window parameters liketoolbar=no, width=700, height=600which creates small fixed-size popup windows. These are harder for users to work with since they can't be resized freely, don't appear in the tab bar, and feel dated compared to just opening a normal browser tab.What is the New Behavior?
Instead of opening restricted popup windows, links now open in regular browser tabs. Users can resize, bookmark, and use browser navigation on these pages like any other tab.
Before: Files and attachments opened in small 700x600 popup windows
After: Files and attachments open in normal browser tabs
What steps should a reviewer take to test this?
Automated Testing & Documentation
No new tests needed - this is a UX improvement that doesn't change functionality. The files still open and display the same content, just in a standard tab instead of a popup.
Other information
I left
openPopUp()andopenActionsPopup()alone for now since those actually write HTML content to the new window rather than just navigating to a URL. Converting those would require a different approach (maybe rendering to an inline modal or a dedicated page).