Migrate assignment summary page to react-table v8 #7630

freyazjiner · 2025-08-04T16:12:19Z

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)
This PR updates the AssignmentSummaryTable to use React Table v8.

Screenshots of your changes (if applicable)

Associated documentation repository pull request (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)	X

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

coveralls · 2025-08-13T04:38:22Z

Pull Request Test Coverage Report for Build 17030526515

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

53 of 79 (67.09%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.04%) to 91.848%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/javascript/Components/assignment_summary_table.jsx	42	68	61.76%

Files with Coverage Reduction	New Missed Lines	%
app/javascript/Components/assignment_summary_table.jsx	1	59.52%

Totals
Change from base Build 16980327805:	-0.04%
Covered Lines:	42121
Relevant Lines:	45145

💛 - Coveralls

david-yz-liu

@freyazjiner nice work, I left a few comments mainly focused on the implementation.

david-yz-liu · 2025-08-15T00:24:20Z

app/javascript/Components/Helpers/table_helpers.jsx

+    cell: info => {
      let marking_state = "";
-      switch (row.original.marking_state) {
+      switch (info.row.original.marking_state) {


Call the parameter props, not info (to match the other examples we have).

Also, given the use of the accessorKey, it shouldn't be necessary to access props.row.original.marking_state, instead you should be able to use props.getValue().

david-yz-liu · 2025-08-15T00:24:32Z

app/javascript/Components/Helpers/table_helpers.jsx

        default:
          // should not get here
-          marking_state = row.original.marking_state;
+          marking_state = info.row.original.marking_state;


same comment as above

david-yz-liu · 2025-08-15T00:30:20Z

app/javascript/Components/table/table.jsx

                  })}
                </div>
+                {row.getIsExpanded() && (
+                  <tr>


I don't really think it makes sense to use <tr>/<td> elements here, since this isn't a "real" table. Instead, please just use a <div> that wraps the subcomponent, e.g.

<div>{renderSubComponent({row})</div>

Unless that looks bad for some reason (provide screenshot).

david-yz-liu · 2025-08-15T00:30:52Z

app/javascript/Components/assignment_summary_table.jsx

+const renderSubComponent = ({row}) => {
+  return (
+    <div>
+      <h4>{I18n.t("activerecord.models.ta", {count: 2})}</h4>


This count should be equal to the number of graders, not a hard-coded number.

david-yz-liu · 2025-08-15T00:33:58Z

app/javascript/Components/assignment_summary_table.jsx

+            );
+            return (
+              <a href={path}>
+                {info.row.original.group_name}


As above, use the parameter name props throughout, and getValue( as well.

app/javascript/Components/assignment_summary_table.jsx

david-yz-liu · 2025-08-15T00:39:09Z

app/javascript/Components/assignment_summary_table.jsx

+        size: 100,
+        enableResizing: true,
+        cell: value => value.getValue(),
+        meta: {className: "number"},


This isn' exactly correct. Look at the values passed in the criteriaColumns state; there are additional attributes that are meant to be used here.

To see the difference in the UI, assign graders to specific criteria (and students) and then login as one of the graders and visit the table. Criteria that haven't been assigned should be greyed out due to the styling.

david-yz-liu · 2025-08-15T00:40:07Z

app/javascript/Components/assignment_summary_table.jsx

+      header: () => I18n.t("activerecord.models.extra_mark.other"),
+      size: 100,
+      enableResizing: true,
+      cell: info => info.getValue(),


Even though you translated this correctly, I think this is a case where the original code was redundant, so you can just delete this cell attribute and use the react table default.

david-yz-liu · 2025-08-15T00:49:58Z

app/javascript/Components/assignment_summary_table.jsx

+      size: 32,
+      cell: ({row}) => {
+        return row.getCanExpand() ? (
+          <button


This is pretty good, but can be improved:

All styles should go into a CSS file, not be set here

The spread operator (...) is unnecessary. Just do onClick={...} etc. to pass attributes to the button.

Rather than using separate unicode characters, take a look at how the original did it:

component: https://github.com/TanStack/table/blob/v6/src/defaultProps.js#L237-L239

CSS: https://github.com/rdubigny/react-table-v6/blob/8cffee96fad788484edf512aa8fe4fab176e803b/src/index.styl#L155-L175

You should be able to copy the styling into our CSS file as well to get the triangle icon to show up.

david-yz-liu · 2025-08-15T00:52:13Z

app/javascript/Components/assignment_summary_table.jsx

+  getColumns = () => {
+    const columnHelper = createColumnHelper();
+
+    const expanderColumn = columnHelper.display({


Overall, this new column definition is good but should be available more generally, not just in this file. Please (1) move it into the table.jsx file and (2) modify the defintion of Table itself so that if a subcomponent renderer prop is passed in (i.e., not undefined), then this column is automatically prepended to the columns passed in as props. This will generalize your work to all tables with subcomponents.

david-yz-liu · 2025-08-15T17:38:11Z

app/javascript/Components/Helpers/table_helpers.jsx

-    Header: I18n.t("activerecord.attributes.result.marking_state"),
-    accessor: "marking_state",
-    Cell: row => {
+    header: I18n.t("activerecord.attributes.result.marking_state"),


shouldn't this have a () => ?

david-yz-liu · 2025-08-15T19:48:17Z

app/javascript/Components/__tests__/assignment_summary.test.jsx

+    });
+
+    await waitFor(() => {
+      expect(screen.getByText(I18n.t("activerecord.models.ta", {count: 2}))).toBeInTheDocument();


This count should also not be hard-coded

david-yz-liu · 2025-08-15T19:52:40Z

app/javascript/Components/table/table.jsx

              <div className="rt-tr-group" role="rowgroup" key={row.id}>
                <div className="rt-tr -odd" role="row" key={row.id}>
                  {row.getVisibleCells().map(cell => {
+                    const metaClass = cell.column.columnDef.meta?.className || "";


This is good, but one of the things you should notice about the criteria column data is the headerClassName field. This should also be handled by the Table component.

david-yz-liu · 2025-08-15T19:53:15Z

app/javascript/Components/assignment_summary_table.jsx

+import {createColumnHelper} from "@tanstack/react-table";
+import Table from "./table/table";
+
+const renderSubComponent = ({row}) => {


Move this to below the AssignmentSummaryTable class. In general, I prefer having helpers below the main function.

david-yz-liu · 2025-08-15T20:09:51Z

app/javascript/Components/Helpers/table_helpers.jsx

+    accessorKey: "marking_state",
+    size: 100,
+    enableResizing: true,
+    cell: props => {


There's a problem with the filtering for the column: the options show the "raw" string values (in_progress, not_collected, etc.) instead of the internationalized strings.

To fix this, you can use the same approach that @ch-iv took for the instructor table: post-process the marking state values from the fetched data to transform the raw strings into the I18n.t versions. See https://github.com/MarkUsProject/Markus/blob/master/app/javascript/Components/instructor_table.jsx#L58-L79

david-yz-liu · 2025-08-15T20:14:07Z

app/javascript/Components/Helpers/table_helpers.jsx

-    Header: I18n.t("activerecord.attributes.result.marking_state"),
-    accessor: "marking_state",
-    Cell: row => {
+    header: I18n.t("activerecord.attributes.result.marking_state"),


Oh right, overall you should not change this function (since it's also used in the SubmissionTable component, which is on an older version of React table). Instead, you can make a copy of the function and move it directly into the assignment_summary file.

david-yz-liu

@freyazjiner nice work! I just left a few small comments

david-yz-liu · 2025-08-18T01:32:36Z

Changelog.md

 - Updated Python autotest script file to include example of using `pytest.mark` to customize test marks (#7597)
 - Refactor `Grader` table in `Users` to use `@tanstack/react-table` v8 (#7598)
 - Updated `dependabot.yml` to add React dependency group and increase, remove old webpack ignored versions, and remove open PR limits (#7627)
+- Updated the assignment summary table to use `@tanstack/react-table` v8 (#7630)


Please do a merge from master. When you do, you'll see that there's a new, much smaller "unreleased" section (due to a new release being made). Add this entry to the new section.

david-yz-liu · 2025-08-18T01:36:58Z

app/javascript/Components/assignment_summary_table.jsx

+        id: col.id,
+        header: () => col.Header,
+        size: col.size || 100,
+        meta: {headerClassName: col.headerClassName},


you should include both className and headerClassName here

david-yz-liu

Nice work, @freyazjiner!

freyazjiner added 11 commits August 4, 2025 12:09

migrate to v8

97a39b2

migrate to v8

2212ab1

change the button

267dbc2

resize the columns

a1562ca

more changes for assignment summary table to react table v8

aa9b4ab

add tests for the subcomponent behaviour

f19c9fb

move toggleShowInactiveGroups out of fetchData

09b6e7c

remove the test first to check the table's behavior first

eadbff5

add tests for the subcomponent

325a19e

update columnFilters

6e17fe9

fixed columnFilters

cc329a3

updated changelog

0a12bb8

freyazjiner requested a review from david-yz-liu August 13, 2025 04:42

Merge branch 'master' into react-table-v8-assignment-summary

ab72a2d

david-yz-liu reviewed Aug 15, 2025

View reviewed changes

freyazjiner added 4 commits August 15, 2025 11:11

1

9c1063d

update

100c054

add back the column filter

7a34f23

modify test

4e2d6a9

freyazjiner requested a review from david-yz-liu August 15, 2025 16:16

david-yz-liu reviewed Aug 15, 2025

View reviewed changes

f

56bd226

freyazjiner requested a review from david-yz-liu August 17, 2025 05:08

david-yz-liu reviewed Aug 18, 2025

View reviewed changes

freyazjiner added 4 commits August 17, 2025 23:40

1

4a418e3

changelog

4640c28

q

e8c4aa3

changlog

c43a0f9

freyazjiner requested a review from david-yz-liu August 18, 2025 04:34

david-yz-liu approved these changes Aug 18, 2025

View reviewed changes

david-yz-liu merged commit 7398e68 into MarkUsProject:master Aug 18, 2025
7 checks passed

david-yz-liu added this to the v2.8.1 milestone Aug 18, 2025

donny-wong pushed a commit to donny-wong/Markus that referenced this pull request Sep 10, 2025

Migrated assignment summary table to react-table v8 (MarkUsProject#7630)

2686eeb

freyazjiner deleted the react-table-v8-assignment-summary branch January 5, 2026 18:35

Migrate assignment summary page to react-table v8 #7630

Migrate assignment summary page to react-table v8 #7630

Uh oh!

Conversation

freyazjiner commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Type of Change

Checklist

Questions and Comments

Uh oh!

coveralls commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17030526515

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

freyazjiner commented Aug 4, 2025 •

edited

Loading

coveralls commented Aug 13, 2025 •

edited

Loading