Added loading icon for instructor table #7602

chickenwaddle77 · 2025-07-29T08:00:52Z

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.)

Added loading icon for custom Table prop when front-end is fetching.

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)	X
♻️ 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)

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.)

chickenwaddle77 · 2025-07-29T08:01:40Z

@david-yz-liu Hi David, do you mind looking at my current loading icon to see if my implementation has any issues? Thank you!

coveralls · 2025-07-29T08:26:40Z

Pull Request Test Coverage Report for Build 17863161339

Details

10 of 11 (90.91%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 91.793%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/javascript/Components/Helpers/table_helpers.jsx	5	6	83.33%

Totals
Change from base Build 17782680830:	-0.03%
Covered Lines:	42204
Relevant Lines:	45251

💛 - Coveralls

david-yz-liu · 2025-07-29T15:37:54Z

Hi @chickenwaddle77, the loading spinner looks okay to me, though what I suggest you do is prepare a few different visual options for your presentation tomorrow and get feedback from the other students about what they think look best

chickenwaddle77 · 2025-07-29T17:05:40Z

Hi @chickenwaddle77, the loading spinner looks okay to me, though what I suggest you do is prepare a few different visual options for your presentation tomorrow and get feedback from the other students about what they think look best

Sounds good I will do that. Thanks!

…sts to check student_table, ta_table, instructor_table

chickenwaddle77 · 2025-08-06T08:02:03Z

@david-yz-liu Hi David, I've been trying to resolve an issue with the NoDataComponent that shows up above my custom spinner for the react v6 table. I've been trying different ways to remove it during loading (and only keep it when no data is available). Do you know how to fix this? Thank you!

david-yz-liu · 2025-08-06T16:35:19Z

Hi @chickenwaddle77, you indeed encountered an interesting problem. How I investigated this was to look directly at the relevant source code for React Table v6: https://github.com/TanStack/table/blob/v6/src/index.js#L850-L851.

It looks like you'll need to also provide a noDataProps function as well; this function is being called on the "resolved state" (a combination of the state and props passed to the ReactTable component). Try to write a function for noDataProps that extracts just the loading property from the state and returns it in a new object (return {loading: state.loading}).

Then in the NoDataComponent, extract loading directly, not props: customNoDataComponent({children, loading}) { ... }.

… table-loading merge with upstream

chickenwaddle77 · 2025-08-06T16:51:20Z

Hi @chickenwaddle77, you indeed encountered an interesting problem. How I investigated this was to look directly at the relevant source code for React Table v6: https://github.com/TanStack/table/blob/v6/src/index.js#L850-L851.

It looks like you'll need to also provide a noDataProps function as well; this function is being called on the "resolved state" (a combination of the state and props passed to the ReactTable component). Try to write a function for noDataProps that extracts just the loading property from the state and returns it in a new object (return {loading: state.loading}).

Then in the NoDataComponent, extract loading directly, not props: customNoDataComponent({children, loading}) { ... }.

Thank you David, I will read this and implement the changes!

chickenwaddle77 · 2025-08-15T05:08:47Z

Hi @david-yz-liu, I was wondering if you can help me with the student_table.test.jsx file and the For each StudentTable's loading status -> shows loading spinner when data is being fetched test case. I can see that the spinner is displaying in the front-end, and the tests for the custom react table is passing too (ta_table.test.jsx, instructor_table.test.jsx) but it is just the test in this test file that doesn't pass.

I've tried to:

change the ariaLabel="grid-loading" to aria-label="grid-loading". Tried using data-testid.
Used different testing functions getByTestId, getByRole
wrap the render in an act statement that was suggested by Jest
explicitly setting loading to be true when rendering.
Temporarily installed a library to view snapshot HTML code

I was wondering if the default table has something different that Jest can't see when testing? Thanks!

david-yz-liu · 2025-08-15T19:41:16Z

@chickenwaddle77 the current CI run doesn't illustrate the error you're referring to (look at https://github.com/MarkUsProject/Markus/actions/runs/16983377732/job/48147695080?pr=7602). Fix the error first and show your best effort to get the test to pass, and ensure that the error you're seeing locally also appears on the GitHub Actions CI.

…nents

chickenwaddle77 · 2025-08-20T20:18:21Z

@david-yz-liu Hi David, I've completed the Jest testing for the Loading components and fixed the errors. Turns out the error was that ReactTableDefaults is not being used in Jest so it does not configure a custom LoadingComponent for us. So I manually defined one in the testing files (imported from table_helper) and then made a condition to use the passed in prop in the testing environment (else use the default).

There are also a few tables that are not exported but they use a factory method to construct them. Do you also want me to test those? Thanks!

david-yz-liu · 2025-08-21T01:40:28Z

compose.yaml

      - localhost:host-gateway
    ports:
      - '3000:3000'
+      - '3336:3336'


I'm not sure what purpose this is serving, but it doesn't belong in this PR

david-yz-liu · 2025-08-21T01:41:11Z

jest.config.js

  // timers: "real",

  // A map from regular expressions to paths to transformers
-  transform: {"\\.[jt]sx?$": "babel-jest"},


This shouldn't be changed. If you got a compilation error with the new package, add it to transformIgnorePatterns.

I think you missed this comment (what I'm saying is that there should not be any changes to this file).

Oops sorry, I thought I added this change in.

david-yz-liu · 2025-08-21T01:45:36Z

app/javascript/common/react_config.jsx

  FilterComponent: textFilter,
  NoDataComponent: customNoDataComponent,
+  noDataProps: customNoDataProps,
+  // noDataText: customNoDataText,


I think this change is a good idea, please go ahead and delete the code (rather than comment it out)

david-yz-liu · 2025-08-21T01:46:03Z

app/javascript/Components/table/table.jsx

+            (noDataText === "" ? (
+              <div
+                className="flex gap-4"
+                style={{


All style should go in CSS files; you can add classes to these elements to trigger CSS rules

david-yz-liu · 2025-08-21T01:47:31Z

app/javascript/Components/ta_table.jsx

+      <Table
+        data={this.state.data}
+        columns={this.columns}
+        noDataText={this.state.loading ? "" : I18n.t("tas.empty_table")}


This change should be reverted; it shouldn't matter what the noDataText is if this.state.loading is true, since you've modified the code to not display the "no data" component in this case.

david-yz-liu · 2025-08-21T01:52:48Z

app/javascript/Components/student_table.jsx

  render() {
    const {data, loading} = this.state;
+    const effectiveLoadingComponent =
+      process.env.NODE_ENV === "test"


There should not be code that's conditional on the environment. You should remove this code and use a different approach to resolve the problem you are encountering with the tests.

You said:

Turns out the error was that ReactTableDefaults is not being used in Jest so it does not configure a custom LoadingComponent for us.

I'm guessing here you meant that the code in react_config.js. Any code that you want to be executed for the tests can be added to jest_after_env_setup.js.

… table-loading merge with origin

chickenwaddle77 · 2025-08-27T01:18:01Z

Hi @david-yz-liu, sorry I took a bit of time. I was a little confused on how to fix some issues with the Jest tests. Because I deleted this.state.loading ? so then I didn't realize that my implementation of our custom React Table stopped working because it relied on it. So I just realized that was the issue and made my changes. Thanks!

chickenwaddle77 · 2025-08-27T01:49:32Z

Hi @david-yz-liu, I was wondering what I can do about the testing coverage? Thanks!

david-yz-liu

@chickenwaddle77 nice work! I left a few more comments for review; please work on those, but don't worry about the test coverage right now.

david-yz-liu · 2025-08-30T01:05:19Z

Changelog.md

 ### 🔧 Internal changes
 - Updated the assignment summary table to use `@tanstack/react-table` v8 (#7630)
 - Updated Github Actions CI to use cache-apt-pkgs to speed up workflow runs (#7645)
+- Added new loading spinner icon for tables (#7602)


Move this to the "Enhancements" section

Hi David, there was no Enhancements section so I moved it to "New features and improvements." Please let me know if that was what you meant.

david-yz-liu · 2025-08-30T01:07:01Z

app/javascript/Components/Helpers/table_helpers.jsx

 */

+export function customLoadingProp(props) {
+  const {loading} = props;


You can simplify this code by using the same syntax as in other functions below:

export function customLoadingProp({loading}) { ...

david-yz-liu · 2025-08-30T01:08:07Z

jest.config.js

  // timers: "real",

  // A map from regular expressions to paths to transformers
-  transform: {"\\.[jt]sx?$": "babel-jest"},


I think you missed this comment (what I'm saying is that there should not be any changes to this file).

david-yz-liu · 2025-08-30T01:08:48Z

app/javascript/Components/Helpers/table_helpers.jsx

+  return {loading: state.loading, data: state.data};
+}
+
+// export function customNoDataText(props) {


Delete this function since it's unused

david-yz-liu · 2025-08-30T01:10:09Z

app/javascript/Components/course_summaries_table.jsx

@@ -1,5 +1,5 @@
 import React from "react";
-
+import {ReactTableDefaults} from "react-table";


This import is unused and should be removed

david-yz-liu · 2025-08-30T01:10:36Z

app/javascript/Components/notes_table.jsx

 import ReactTable from "react-table";
 import {faPencil, faTrashCan} from "@fortawesome/free-solid-svg-icons";
 import {FontAwesomeIcon} from "@fortawesome/react-fontawesome";
+import {ReactTableDefaults} from "react-table";


Same comment as above

david-yz-liu · 2025-08-30T01:11:42Z

app/javascript/Components/student_table.jsx

              filterable: false,
            },
          ]}
+          noDataText={I18n.t("students.empty_table")}


This line should be moved back to Line 240 (look at where it is in the original code), as there isn't actually a change.

david-yz-liu · 2025-08-30T01:13:29Z

app/javascript/Components/student_table.jsx

+          loading={this.state.loading}
          ref={r => (this.checkboxTable = r)}
-          data={data.students}
+          data={this.state.loading ? [] : data.students}


This change is unnecessary (whether or not the loading spinner appears should depend only on the loading prop, and not the value of the data prop)

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

app/javascript/common/react_config.jsx

  NoDataComponent: customNoDataComponent,
+  noDataProps: customNoDataProps,
+  LoadingComponent: customLoadingProp,
+  loadingText: null,


I don't think this prop (loadingText) is being used anywhere. Assuming this is a change from an earlier version of this PR, you can remove it.

david-yz-liu · 2025-08-30T01:19:47Z

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

        }),
      });
-      render(<StudentTable selection={[]} course_id={1} />);
+      await act(async () => {


I know you mentioned encountering issues with the Jest tests, but this change is suspicious. I am skeptical that this is a good change to make, so please revert it (and the similar change in ta_table.test.jsx). If there are failing tests I'll look into it and give you some advice about next steps.

I think it is working fine without the await act function now. The problem was probably not how the test is checking for components but that ReactTableDefaults was not called in the jest environment. Sorry I didn't think of reverting this change back.

chickenwaddle77 · 2025-09-14T21:27:25Z

I think there is a jest error, I will fix tonight.

chickenwaddle77 · 2025-09-16T15:41:52Z

I was checking out the previous commit head and figuring out what was wrong. I think I accidentally deleted one of the import {ReactTableDefaults} from "react-table"; lines that were used in react_config.jsx file so then the application didn't render properly.

david-yz-liu

@chickenwaddle77 nice work! I made a small styling change, but that's about it. Thanks for all of your work on this, and I appreciate you wrapping this up. 🎉

chickenwaddle77 added 2 commits July 29, 2025 03:52

added loading icon for instructor table

e8a7a85

commented out sleep

581c293

chickenwaddle77 added 3 commits July 30, 2025 15:03

added few spinners to try out

0299525

added loading spinner prop to react v6 library + added sample jest te…

e578cd8

…sts to check student_table, ta_table, instructor_table

Merge branch 'master' into table-loading

4574beb

chickenwaddle77 added 2 commits August 6, 2025 12:45

fixed jest errors

ce4929f

Merge remote-tracking branch 'refs/remotes/origin/table-loading' into…

e766c1d

… table-loading merge with upstream

chickenwaddle77 added 3 commits August 13, 2025 11:06

fixed both loading message and no rows found error

d03b0e5

cleaned the react table, removed console.log messages

38b55e7

fixed tests, working for ta_table and instructor

3e17d98

chickenwaddle77 added 2 commits August 20, 2025 16:11

fixed jest test errors and completed testing suite for exported compo…

4133808

…nents

removed dependency

a552a9d

Merge branch 'master' into table-loading

8319a45

david-yz-liu reviewed Aug 21, 2025

View reviewed changes

chickenwaddle77 added 4 commits August 26, 2025 19:59

made changes from review + fixed test cases

5b7bde4

merged with upstream master

b55dec0

Merge branch 'MarkUsProject:master' into table-loading

0f04253

Merge remote-tracking branch 'refs/remotes/origin/table-loading' into…

635cdd9

… table-loading merge with origin

chickenwaddle77 requested a review from david-yz-liu August 27, 2025 01:49

david-yz-liu reviewed Aug 30, 2025

View reviewed changes

chickenwaddle77 added 2 commits September 14, 2025 16:23

merged with upstream

eda0cee

applied changes requested, refactored code

944092c

chickenwaddle77 added 2 commits September 16, 2025 11:11

removed ununsed act import

33cad90

found error that caused code to break

8034b39

chickenwaddle77 requested a review from david-yz-liu September 16, 2025 17:44

chickenwaddle77 and others added 2 commits September 18, 2025 14:36

Merge branch 'master' into table-loading

abdd1ba

Added basic style for the loading icon

0908856

david-yz-liu approved these changes Sep 19, 2025

View reviewed changes

david-yz-liu merged commit f61daaa into MarkUsProject:master Sep 19, 2025
4 checks passed

		@@ -1,5 +1,5 @@
		import React from "react";

		import {ReactTableDefaults} from "react-table";

Added loading icon for instructor table #7602

Added loading icon for instructor table #7602

Uh oh!

Conversation

chickenwaddle77 commented Jul 29, 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!

chickenwaddle77 commented Jul 29, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17863161339

Details

💛 - Coveralls

Uh oh!

david-yz-liu commented Jul 29, 2025

Uh oh!

chickenwaddle77 commented Jul 29, 2025

Uh oh!

chickenwaddle77 commented Aug 6, 2025

Uh oh!

david-yz-liu commented Aug 6, 2025

Uh oh!

chickenwaddle77 commented Aug 6, 2025

Uh oh!

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

Uh oh!

Uh oh!

david-yz-liu commented Aug 15, 2025

Uh oh!

chickenwaddle77 commented Aug 20, 2025

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!

chickenwaddle77 commented Aug 27, 2025

Uh oh!

chickenwaddle77 commented Aug 27, 2025

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!

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

chickenwaddle77 commented Jul 29, 2025 •

edited

Loading

coveralls commented Jul 29, 2025 •

edited

Loading

chickenwaddle77 commented Aug 15, 2025 •

edited

Loading