✨ Add Tasks tab to the Applications table application details drawer #2123
Conversation
Signed-off-by: EstyBiton <[email protected]>
Signed-off-by: esty <[email protected]> Signed-off-by: EstyBiton <[email protected]>
Signed-off-by: EstyBiton <[email protected]>
Signed-off-by: EstyBiton <[email protected]>
EstyBiton
force-pushed
the
tasks-tab-drawer
branch
from
ed6042b
to
fc085f3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2123 +/- ##
==========================================
+ Coverage 39.20% 41.95% +2.75%
==========================================
Files 146 175 +29
Lines 4857 5635 +778
Branches 1164 1343 +179
==========================================
+ Hits 1904 2364 +460
- Misses 2939 3255 +316
- Partials 14 16 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The work done is quite good. There are some things that need to be changed.
-
On the side bar table, either drop the priority column or make the initial width of the drawer larger. With the current width of the drawer, the action kebab are not visible. The other option is to make the drawer something like 100px wider initially so the task action kebab is shown.
-
When linking to the task details, the URL is breaking. You can follow the same format as
navigateToAnalysisDetails(), but it should really be its own path. -
The status column should use the same column style as the tasks-page (status icon, name, and link). See:
tackle2-ui/client/src/app/pages/tasks/tasks-page.tsx
Lines 244 to 257 in 0cfe60b
-
Need to fix the link that are being generated on task manager table (see specific comment).
I have a few more specific comment in the review.
client/src/app/Paths.ts
Outdated
| @@ -41,7 +41,7 @@ export const DevPaths = { | |||
|
|
|||
| dependencies: "/dependencies", | |||
| tasks: "/tasks", | |||
| taskDetails: "/tasks/:taskId", | |||
| taskDetails: "/tasks/:taskId/:isFApplication", | |||
There was a problem hiding this comment.
This is causing a bad path on the Task Manager page:
| const tableControls = useTableControlProps({ | ||
| ...tableControlState, | ||
| idProperty: "id", | ||
| currentPageItems: currentPageItems, | ||
| totalItemCount, | ||
| isLoading: isFetching, | ||
| selectionState: useSelectionState({ | ||
| items: currentPageItems, | ||
| isEqual: (a, b) => a.name === b.name, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
I think a compact table looks better here. Add prop variant: "compact", and see if you agree.
| modifier="nowrap" | ||
| {...getTdProps({ columnKey: "status" })} | ||
| > | ||
| {task.state} |
There was a problem hiding this comment.
Make this render like on the task manager page -- status icon with a link to view the task details
| item={task} | ||
| rowIndex={rowIndex} | ||
| > | ||
| <Td width={10} {...getTdProps({ columnKey: "taskId" })}> |
There was a problem hiding this comment.
The Td widths are not necessary. All of the patterfly examples put any width values on the Th tags only. Since I don't see any difference with them here or not, easiest is to remove them.
| const [applicationName, setApplicationName] = useState<string | undefined>(); | ||
| const [applicationId, setApplicationId] = useState<number | undefined>(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
@rszwajko Can you have a look at these changes? I don't remember exactly how these components hang together.
Signed-off-by: EstyBiton <[email protected]>
|
@sjd78 Thank you for your review! I’ve made the following updates based on your feedback:
|
Enhancing the application details drawer by adding a "Tasks" tab, displaying a table of tasks associated with the application, implementing filtering, sorting, and pagination functionalities, and including columns for Task ID, Task Kind, Status, and Priority.
Fixes: #1937
screenshot of result:
