[Refactor:InstructorUI] Consolidate Table Sorting#12996
Conversation
automateprojectmangement
Bot
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12996 +/- ##
============================================
- Coverage 21.12% 21.11% -0.01%
Complexity 10114 10114
============================================
Files 279 282 +3
Lines 38125 38231 +106
Branches 578 596 +18
============================================
+ Hits 8053 8074 +21
- Misses 29507 29575 +68
- Partials 565 582 +17
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
typescript is picky about having functions with the same name, so in order to expose sortTableByColumn on window I have to comment out all other sortTableByColumn functions. Everything commented out here will eventually be removed or refactored
... but now I need to change the way that the items inside row elements are found, because being able to toggle columns messes with predefined column indexes
this code only seems to trigger for the section column, but the styling added doesn't actually seem to apply, and anyways this styling doesn't seem meaningful; there is already the Section header, so I see no reason to put "section" as ::before content
This reverts commit c0934e6.
mattLif
left a comment
mattLif
left a comment
There was a problem hiding this comment.
Requested changes are being made in another PR. Approving this one.
NicholasCiuica
moved this from Work in Progress
to Awaiting Maintainer Review
in Submitty Development
I address these changes and the ongoing table highlight issue in my PR #13037 |
jndlansh
left a comment
jndlansh
left a comment
There was a problem hiding this comment.
I approve. Great work @NicholasCiuica !
github-project-automation
Bot
moved this from Awaiting Maintainer Review
to Work in Progress
in Submitty Development
|
@jndlansh I came across an issue where the TableSortManager would emit the restore-table-sort before DOM content was loaded, so I made another commit to reimplement the window.addEventListener on DOMContentLoaded. Do you know of any better way to wait until the table and its contents are loaded on the page before trying to restore their sort, without having to rely on external DOM? Currently my change causes some component tests to fail. |
I've done some research on the topic of making an emit after the DOM content is loaded, and have not found an easy solution to our problem here. I have implemented a hacky fix to our TableSortManager component tests that manually triggers a load event before listening for the emit response, but I do not think this is a good solution. Any suggestions would be much appreciated. |
### Why is this Change Important & Necessary? Fixes #12584, Related to #12996 Should only be merged after #12996 for easier testing and important CSS changes ### What is the New Behavior? Course staff are now properly highlighted on the Simple Grading and Gradeable Details pages. Also, the Gradeable Details page also uses the same blue to highlight the course staff as the rest of the gradeable tables. ### What steps should a reviewer take to reproduce or test the bug or new feature? Please follow the testing procedure on multiple browsers (at least Chromium and Firefox). 1. Go to the main branch 2. Go to the following gradeables, and for each of them you should see that staff are not highlighted in the tables: i. Open a gradeable with student file submission ii. Make a Numeric gradeable with at least on numeric column and at least one text column iii. Make a Checkpoint gradeable with at least one checkpoint column and at least one text column <img width="745" height="311" alt="image" src="https://github.com/user-attachments/assets/817dcf9c-5acd-4f8b-af05-9775335adb2f" /> 6. Now go to this branch and submitty_install_site 7. Open each gradeable that you opened/made on main and you should now see that staff are highlighted in the tables: <img width="738" height="274" alt="Screenshot 2026-07-14 111103" src="https://github.com/user-attachments/assets/19f8d392-7dd7-449f-b009-baecba670e7c" /> EDIT: Please ensure that on the numeric and checkpoint gradeables row highlighting stops at the user's identifying information, so that the highlight doesn't interfere with checkpoint grading cells or numeric/text input cells. e.g. <img width="724" height="244" alt="image" src="https://github.com/user-attachments/assets/9516baba-2bfc-436c-8162-41cdb5966ecc" /> ### Automated Testing & Documentation There are currently no tests that check for highlighted staff rows in the gradeable tables. Should there be?
|
@NicholasCiuica I think we wait for the DOM content to load (Let the browser finish Parsing the HTML) |
automateprojectmangement
Bot
moved this from Work in Progress
to Seeking Reviewer
in Submitty Development
mattLif
left a comment
mattLif
left a comment
There was a problem hiding this comment.
Rereviewed using same pages, and all previous issues have been addressed or are actively being addressed in a new PR. Code still looks good too. Good work Nick, I approve.
Since sorting by section is not currently implemented, I'll remove it for now. This feature will require some DOM manipulation that might be better saved for when more of the table is refactored to Vue.
github-project-automation
Bot
moved this from Seeking Reviewer
to Done
in Submitty Development
5 participants
Why is this Change Important & Necessary?
Related to #12960
I'm going to tackle this issue in parts. This first PR will be consolidating all the table sorting logic, as currently each table requires the declaration of its own sorting function and helper functions, but these share lots of code with each other. This will make it easier to create multisortable columns, but also will make it easier to add new sortable tables.
What is the New Behavior?
Now all tables on submitty that have sortable columns share the same sorting logic, because they all use the new SortableTableHeader Vue component. This makes it easy to add or remove sortable columns from existing tables, or make new sortable tables.
To enable saving table sort between reloads, the new TableSortManager Vue component needs to be added to the same Twig page as the sortable table. The only table that I have enabled this persistent sorting behavior for is the details-table, to match current persistent behavior. When multisortable column behavior is introduced, it will build off of the TableSortManager.
What steps should a reviewer take to reproduce or test the bug or new feature?
Ensure that the sortable table headers still work for all the following tables. Note that some tables now have more sortable columns than before (e.g. docker-table is now sortable by tags and owners).
Log in as instructor:
Log in as superuser:
NOTE: To test sortable columns that may have homogenous data by default (e.g. docker-table tags all being "latest"), you can change the text in the columns using your browser's dev/inspect tools and the sorting should still work.
Please let me know if there are any other sortable tables in Submitty that I missed in this PR, and I will handle them in another PR.
EDIT: I have made a number of naming changes to variables related to column headers and sorting in an attempt to make the code easier to understand and more consistent. Please let me know what you think of these changes. Besides name changes, the majority of the new functionality code is concentrated in the newly created files of this PR.
EDIT2: I have found some examples where database entries are sorted before being sent to the frontend, such as when the Randomize Students option is selected above the details-table for grading. Assessing whether this backend sorting is still needed and making any changes or improvements should be done in another PR.
EDIT3: There are still some tables that use a sortTable() function (the activity dashboard and the content_upload_table) which I may convert in a future PR, depending on how they are sorted and if they would benefit from the new components introduced in this PR.
EDIT4: I've added also a slight CSS change that gives active column headers a text-shadow ("double-bold") effect. This is to stand out against the regular bold of the column headers. I believe an effect like this was intended initially, since active-headers were explicitly made bold with CSS, but table headers are already implicitly bold by default.
Inactive:
Active (being sorted by):
Automated Testing & Documentation
Cypress tests that currently test table sorting should still work.
The new Vue Components have their own Cypress tests. Thank you @jndlansh !!!
Should this feature be documented on submitty.org? Let me know.