Skip to content
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

Resolve loading indicator issues in grading table #3064

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

josh1248
Copy link
Contributor

@josh1248 josh1248 commented Oct 27, 2024

Resolves #3048

Good to do before tackling #2814 so that loading can be more explicity indicated.

This PR resolves the loading indicator bug in the grading table, whereby a request for a new submissions page does not properly get reflected as "loading" until backend data arrives. This problem likely arised due to the use of a deprecated API from AGGrid.

To accompany this change, additional measures have been added to prevent users from spamming new requests while awaiting for data - this disabling is now made possible by the proper loading state.

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

  • In the backend, set a high number of students in priv/repo/seeds.exs, example 20000 students and 100 questions. It should be enough such that backend fetching takes at least 1 second.
  • Go to the grading table and attempt to load the next page. Notice that the loading logo stays until new data is received.
  • Attempt to "spam" the next page button. Notice that you are now unable to do so due to the page change locking while data is being loaded.
  • Attempt to continuously hit the refresh button. Notice that it is also disabled while data is being loaded.
  • Note that this lock is not applied on the dropdown list as there is no HTML API to disable it easily - but this shouldn't be an issue since it cannot be repeatedly clicked in a short time.

Checklist

  • I have tested this code
  • I have updated the documentation

Current implementation uses the outdated api.showLoadingOverlay(), which
causes issues in loading as the loading logo disappears even before
the data arrives. This commit uses the current method of using the
"loading" field in the AGGrid component, which solves the loading
mismatch issue.
@josh1248 josh1248 requested a review from RichDom2185 October 27, 2024 14:06
@josh1248 josh1248 marked this pull request as ready for review October 27, 2024 14:06
@josh1248
Copy link
Contributor Author

ready for review!

@coveralls
Copy link

coveralls commented Oct 27, 2024

Pull Request Test Coverage Report for Build 11567897786

Details

  • 0 of 13 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 31.325%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pages/academy/grading/Grading.tsx 0 2 0.0%
src/pages/academy/grading/subcomponents/GradingSubmissionsTable.tsx 0 11 0.0%
Totals Coverage Status
Change from base Build 11566681709: -0.01%
Covered Lines: 4833
Relevant Lines: 14553

💛 - Coveralls

Copy link
Contributor

@sayomaki sayomaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a preliminary review of this PR, and it looks great! Thanks for the improvements!

Just to clarify but from what I see this does not seem to fully resolve #3048, does it?

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also pushed some cosmetic changes for better UI and code quality, LGTM, thanks!

@RichDom2185 RichDom2185 enabled auto-merge (squash) October 29, 2024 05:32
@RichDom2185 RichDom2185 merged commit 99e54ed into source-academy:master Oct 29, 2024
6 checks passed
@josh1248
Copy link
Contributor Author

Did a preliminary review of this PR, and it looks great! Thanks for the improvements!

Just to clarify but from what I see this does not seem to fully resolve #3048, does it?

Hi sayo, apologies for the late reply - it does solve refresh issues for next page / previous page / filters, but not for the dropdown menus for page size or grouping. This is because there wasnt an API to disable them - and I thought it wouldn't be an issue since you couldn't spam backend requests through the dropdown menus easily. Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grading table refresh button bugged
4 participants