-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Grading Overview: TS/Tremor -> AG/Blueprint Migration #2893
Grading Overview: TS/Tremor -> AG/Blueprint Migration #2893
Conversation
Switch branch names as changes are more than just filterable columns
Merge changes cause I forgot to switch branch and this is easier
I need to stop not switch branch
…f tanstack and tremor table
…, removal of old code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, looking forward for the changes! For now, I have some preliminary comments below:
Thanks for working on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the resolution of merge conflicts, I went to prioritise your changes over the recently merged PRs, thus the current version does not support team assessments (in fact, some of the other files give a compile error).
Can you help take a look? I've attached the fields that were modified by team assessment PR as a code comment.
The basic idea is that now there will be an XOR constraint (either studentName
will be populated in case of an individual assessment, or studentNames
for team assessment – same for studentUsername
and studentUsernames
). Thus the table display will fallback to joining the studentNames
/studentUsernames
field if the other one is empty.
Refer to #2548 for more details.
Please contact me via Telegram if you require any clarifications.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for merging up with most of the conflicts, I'll let you know if I need help with this one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged the changes for this in the latest commit, but I'm unsure if the data shown is correct. I have messaged you on this.
src/commons/grading/GradingFlex.tsx
Outdated
@@ -0,0 +1,56 @@ | |||
declare const twJustifyContentValues: readonly ["justify-start", "justify-end", "justify-center", "justify-between", "justify-around", "justify-evenly"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I check what is this used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think I copied it blindly to avoid errors, will look through as well with the other next comment when I get around to looking at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed in the latest commit, it can be resolved once you are ok with it.
src/commons/grading/GradingFlex.tsx
Outdated
display: "flex", | ||
justifyContent: | ||
(justifyContent === "justify-start" | ||
? "flex-start" | ||
: justifyContent === "justify-end" | ||
? "flex-end" | ||
: justifyContent === "justify-center" | ||
? "center" | ||
: justifyContent === "justify-between" | ||
? "space-between" | ||
: justifyContent === "justify-around" | ||
? "space-around" | ||
: justifyContent === "justify-evenly" | ||
? "space-evenly" | ||
: "" | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we are mapping Tailwind values to actual styles – what's the need for this? Can we just map input props directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just being lazy and trying to avoid touching existing props but I can go through all of it again when I work on it to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed in the latest commit, it can be resolved once you are ok with it.
src/commons/grading/GradingFlex.tsx
Outdated
); | ||
} | ||
|
||
export default GradingFlex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line at the end of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed in the latest commit, it can be resolved once you are ok with it.
src/commons/grading/GradingText.tsx
Outdated
); | ||
} | ||
|
||
export default GradingText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on newline at EOF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed in the latest commit, it can be resolved once you are ok with it.
src/commons/sagas/BackendSaga.ts
Outdated
@@ -466,6 +484,7 @@ function* BackendSaga(): SagaIterator { | |||
gradedFilter, | |||
pageParams, | |||
filterParams | |||
// sortedBy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be used/removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be used for backend column sorting onced implemented, commented for now in case it causes errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been uncommented in the latest commit and the various issues that comes with it has been fixed.
does this change have the edge case bugs in #2812? seems likely that aggrid's way of doing things over tanstack could help to curb the forceful frontend filtering and solve the issue. |
It does indeed have that case as the way data is handled is not under aggrid, but I will provide a (rough frontend) fix it in the next commit. Edit: The changes have been made, and the second issue in the PR will currently remove the filters for Attempting/Attempted (basically those that aren't Submitted) when selecting ungraded in what to view. |
…s on selecting ungraded, some refactoring
I'm gonna make some minor improvements, hence marking as draft. Thanks for the work so far! |
…nto pr/InfinityTwo/2893
Use dependency injection to avoid needing a hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nits :)
* Simplify props * Remove unneded export
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot! Sorry for the delayed review…
Description
This is a major update to the Grading Overview page which migrates from Tanstack Table and Tremor UI to AG Grid and Blueprint JS. It also introduces a few new features that change the user workflow.
Summary
Preferably merged with backend #1107
Frontend does not error without it, but sorting columns will not sort anything.
Part of Migrate all tables to TanStack from AG Grid #2525, Remove Tremor UI library #2849 and AY2023/24 Sem 1 Avenger feedback on Grading Table #2665.
Resolves Grading UI/UX: Grading Table is not Responsive #2804 and Edge case bugs introduced through transfer of pagination of grading submissions from frontend to backend. #2812.
Adds onto Implement isGradingPublished (plus related features) #2856.
Note
For future Avengers and Developers, we would like you to feedback and think about these aspects for future development
Please give your feedback in the latest Avenger feedback form/issue tracker or feel free to submit a new issue tracker. Thank you!
Features/Changes Added
Feature/Change 1 - Migration of Table
Part of #2525 and #2849. Adds onto #2856.
All components under Grading Overview will have their components migrated from Tanstack Table and Tremor UI to AG Grid and Blueprint JS. This also means some existing features are modified and may not work the same as before. I have tried my best to keep the table looking the same as before, less the changes made as new features. In #2856, Publish and Unpublish grading buttons were added. I've changed it to become icons to better suit the table. The Unpublish icon can be seen in the picture below, with the first row with the grading icon. The publish icon is at the second row of icons, the last icon.
Feature/Change 2 - Grading/Filter Mode
The default workflow is grading mode. Click on the Grading Mode/Filter Mode button beside the search bar.
In Grading mode, the entire row of each submission (except actions) will be clickable, which navigates the user straight to the submission grading page. This was added as the expected workflow for new users (avengers) was to be able to enter the grading of that submission when clicked, instead of filtering it. The Grading action icon will be removed to declutter the table in this mode.
In Filter Mode, it works the same as the previous table. When you click on any cell that allows filtering, it will filter by the value of that cell throughout the table. The Grading action button will be added back here to avoid the friction of going back to Grading Mode.
Besides the button to differentiate Grading/Filter Mode, when you hover over filterable cells, in Grading Mode, individual cells will not be highlighted/underlined when hovered but in Filter Mode, it will do so.
Filter Mode
Grading Mode
Feature/Change 3 - Customer Column Headers - Sorting & Hideable Columns
Somewhat part of #2665.
On the top of each column header, there are two buttons on the right. The first button allows you to sort in ascending/descending order. The workflow is don't sort by default, click to sort by ascending, click again to sort by descending, and click again to not sort it.
Note that the sorting is done on the backend with this PR #1107
The second button allows you to hide the respective columns. This allows you to view the table with less clutter, and it helps with devices that have a smaller screen.
Custom Headers with filtered columns and cell
Other Minor Changes
When filtering or searching for new queries, the table will not load in the new data if another query has been made. On the shell, it still queries for the data in the backend and receives every new query like the previous table as this is a purely frontend change.
I have also added a loading animation to show that it's retrieving the requested data. It shows the animation on the first query and hides it when the most recent data from the most recent query has been received.
The table will also become uninteractable (filtering will still be interactable) when querying for the data.
Tl;dr of how it works: when a query is sent, the counter increments by 1. When a query is received, the counter decrements by 1 or stays at 0 if already at 0. If the counter !== 0, then the loading is shown. In certain rare cases, it may not show the loading overlay (still can't figure out why).
Type of change
How to test
Head over to the Grading page and explore it over there.
Checklist