-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat(KFLUXUI-125): allow sorting pipeline runs by status and type #68
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 80.56% 80.67% +0.11%
==========================================
Files 551 555 +4
Lines 21544 21609 +65
Branches 5383 5124 -259
==========================================
+ Hits 17356 17434 +78
+ Misses 4162 4150 -12
+ Partials 26 25 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
31eb20c
to
25e824c
Compare
/retest |
Overall LGTM. I still need to take a closer look and test the functionality live before final review. |
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
LGTM |
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.
Component Details page and Commits details page also has pipeline runs list page. WE will need to update the same filters their also.
src/components/SnapshotDetails/tabs/SnapshotPipelineRunsList.tsx
Outdated
Show resolved
Hide resolved
src/components/PipelineRun/PipelineRunListView/PipelineRunsListView.tsx
Outdated
Show resolved
Hide resolved
@sahil143 Thanks, did not notice pipeline runs were in the Commit details page as well. By Component Details page, do you mean the |
@sahil143 please check if decomposition like this is okay, so I can work on unit test |
c48ef62
to
292097d
Compare
292097d
to
a75b30a
Compare
using useReducer caused the search input to be buggy in some scenarios
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
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
UI is working as expected. Just left two comments regarding missing comment and the reason of one approach.
if (filterFn && !filterFn(item)) { | ||
return acc; | ||
} |
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.
Why do we need this if statement with you are always returning acc
?
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.
We are always returing acc
but the difference is whether the key counter goes up or not. By returning early when filtered, it will stay the same
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.
But you only modify acc
when there is key
inside validKeys
, if not you return acc
anyway.
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 is possible that we don't want to count some items, even if key
is inside validKeys
. For example, validKeys
might be ["build", "release", "test"]
, pipeline run is of type "build"
, but we want to ignore pipeline runs with name "testing-pipeline"
.
src/components/PipelineRun/PipelineRunListView/__tests__/PipelineRunListView.spec.tsx
Outdated
Show resolved
Hide resolved
97f364c
to
0621bee
Compare
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
7acb2a0
to
40e0b3d
Compare
Fixes
https://issues.redhat.com/browse/KFLUXUI-125
Description
Before, in the application's activity tab, it was only possible to sort pipeline runs by status, and in the snapshot's pipeline runs by type. Now, it's possible to sort by both of these attributes in either of the page, making it more consistent.
Type of change
Screen shots / Gifs for design review
How to test or reproduce?
Go to the activity tab for any application and view the pipeline runs. To view the snapshots pipeline runs, click on any pipeline run from the previous page, click on the snapshot and select the pipeline runs tab.
Browser conformance: