-
Notifications
You must be signed in to change notification settings - Fork 318
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
Add paging on jobs panel #2852
Add paging on jobs panel #2852
Conversation
Signed-off-by: sharpd <davidsharp7@gmail.com>
✅ Deploy Preview for peppy-sprite-186812 canceled.
|
Signed-off-by: sharpd <davidsharp7@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2852 +/- ##
=========================================
Coverage 84.74% 84.74%
Complexity 1456 1456
=========================================
Files 253 253
Lines 6562 6563 +1
Branches 305 305
=========================================
+ Hits 5561 5562 +1
Misses 850 850
Partials 151 151 ☔ View full report in Codecov by Sentry. |
Signed-off-by: sharpd <davidsharp7@gmail.com>
I think the duration should maybe be |
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 👍
Signed-off-by: sharpd <davidsharp7@gmail.com>
Yeah agree - also added the 'New' state as well. |
useEffect(() => { | ||
fetchRuns(job.name, job.namespace) | ||
fetchRuns(job.name, job.namespace, 10, 0) | ||
fetchJobTags(job.namespace, job.name) | ||
}, [job.name]) |
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.
Shouldn't we be setting this to a variable unless I'm missing something to page 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.
It's because the "runs" are used by the job detail page so the thinking being that you pull the latest 10 runs so that job details works and then control all the paging from "Runs". Think there is a similar thing with "Dataset Versions" as that's pulled in the top level component and pushed downwards.
But in terms of having the offset and limit as a var then sure no problem. I've moved some stuff around so all of the work happens in JobDetailsPage.
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
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 pulled down your code and the one thing I'm seeing is that the loading state is wiping out the entire page for loading the runs instead of just the run table. Would it be possible to move the loading rendering detection up the chain and only display a loading indicator for this subsection?
Yeah funnily enough that's part of the reason I split the workload between Runs and JobDetailsPage but agree it's not the best user experience to refresh everything - I will test it on a larger workload as the toy dataset won't cover these kind of use cases. |
Signed-off-by: sharpd <davidsharp7@gmail.com>
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 the updates @davidsharp7
Signed-off-by: sharpd <davidsharp7@gmail.com>
Problem
Currently we pull back the last 100 runs of a job. This PR is to allow for paging so that users can easily scroll through the
runs 10 at a time.
I've also added the total run count to the returned JSON payload on this end point
I will do the PR for dataset versions in due course.
Closes: #2614
Solution
Implement paging similar to that on the sidebar datasets/jobs pages.
One-line summary:
Job level paging of runs.
Checklist
CHANGELOG.md
(Depending on the change, this may not be necessary)..sql
database schema migration according to Flyway's naming convention (if relevant)