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

fix: reset InteractiveTable pagination when filters applied [ET-183] [ET-121] #9413

Merged
merged 2 commits into from
May 23, 2024

Conversation

EmilyBonar
Copy link
Contributor

@EmilyBonar EmilyBonar commented May 22, 2024

Ticket

ET-183
ET-121

Description

There were pagination problems occurring in the following situation:

  1. user is on page N (N > 1) of an InteractiveTable
  2. user applies table filter that makes the total number of pages < N, but which should still have entries
  3. table appears empty

This ticket fixes the problem by resetting the table to the first page when filters are applied.

Test Plan

  1. Find an InteractiveTable with > 10 elements where at least one column that can be filtered on has a mix of values
    • I suggest the Experiment Trials table on an errored multitrial experiment, those generally have a mix of completed and errored trials
  2. Go to the last page
  3. Filter the table by the aforementioned column
  4. The table should reset to the first page

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Copy link

netlify bot commented May 22, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 5da106e
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/664f5d65235d9b00088227ee
😎 Deploy Preview https://deploy-preview-9413--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 4.25532% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 43.99%. Comparing base (860f6a8) to head (5da106e).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9413      +/-   ##
==========================================
- Coverage   51.22%   43.99%   -7.24%     
==========================================
  Files         747      423     -324     
  Lines      110720    70811   -39909     
  Branches     2778     2778              
==========================================
- Hits        56718    31152   -25566     
+ Misses      53827    39484   -14343     
  Partials      175      175              
Flag Coverage Δ
harness ?
web 44.34% <4.25%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
webui/react/src/pages/Admin/UserManagement.tsx 83.39% <100.00%> (ø)
webui/react/src/pages/Templates/TemplatesList.tsx 0.00% <0.00%> (ø)
...t/src/pages/ExperimentDetails/ExperimentTrials.tsx 12.57% <0.00%> (+0.04%) ⬆️
webui/react/src/components/ModelRegistry.tsx 0.00% <0.00%> (ø)
.../pages/ExperimentDetails/ExperimentCheckpoints.tsx 13.52% <0.00%> (+0.03%) ⬆️
webui/react/src/pages/ExperimentList.tsx 0.00% <0.00%> (ø)
webui/react/src/components/TaskList.tsx 0.00% <0.00%> (ø)

... and 324 files with indirect coverage changes

Copy link
Contributor

@johnkim-det johnkim-det left a comment

Choose a reason for hiding this comment

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

I think some of the places we're using InteractiveTable already include a tableOffset update in the handler passed to TableFilterDropdown's onFilter. Would making sure that all of those handlers include that update solve this issue? I think I'd prefer that approach if so.

Copy link
Contributor

@johnkim-det johnkim-det left a comment

Choose a reason for hiding this comment

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

LGTM

@EmilyBonar EmilyBonar merged commit ba31f03 into main May 23, 2024
83 of 97 checks passed
@EmilyBonar EmilyBonar deleted the emilybonar/interactive-table-pagination branch May 23, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants