-
Notifications
You must be signed in to change notification settings - Fork 25
DEVPROD-1976: Migrate execution tasks table to LeafyGreen table #2302
DEVPROD-1976: Migrate execution tasks table to LeafyGreen table #2302
Conversation
1 failed and 1 flaky tests on run #16220 ↗︎
Details:
Review all test suite changes for PR #2302 ↗︎ |
meta: { | ||
search: { | ||
"data-cy": "task-name-filter-popover", | ||
placeholder: "Task name regex", | ||
}, | ||
}, |
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.
Should this be removed since task name isn't a searchable column for the execution tasks table? Or will it be used later?
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.
this column definition will be reused for the upcoming tables, and they'll be shown in those tables! (initially i wrote these changes in one big PR which got split up now into 3 PRs, so that's why they are here 😅)
meta: { | ||
search: { | ||
"data-cy": "variant-filter-popover", | ||
placeholder: "Variant name regex", | ||
}, | ||
}, |
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.
Same here
containerRef: tableContainerRef, | ||
data: executionTasksFull ?? [], | ||
defaultColumn: { | ||
enableColumnFilter: false, |
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.
The current implementation allows multisort but it's missing here. You can check out the tests table for an example!
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.
Ok I just noticed that I don't think this works currently 🤣 do you think we should just omit multisort altogether?
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.
added it back! i think it works now...
meta: { | ||
treeSelect: { | ||
"data-cy": "status-filter-popover", | ||
options: statusOptions, | ||
}, | ||
}, |
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.
Similarly, the current table doesn't allow filtering so this may be superfluous
meta: { | ||
treeSelect: { | ||
"data-cy": "base-status-filter-popover", | ||
options: baseStatusOptions, | ||
}, | ||
}, |
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.
Same as above
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.
💅
failing test just seems like a flaky project settings test, will merge |
DEVPROD-1976
PR is not actually that big, most changes are from regenerating snapshots. True PR size is about ~400 lines.
Description
This PR updates
ExecutionTasksTable
to use LeafyGreen table. I will be opening two more PRs for the remaining tables.Background Info
Currently Execution Tasks, Downstream Projects, and Patch Tasks all share the same table instance. I've decided to split them up into separate tables because it is difficult to reconcile their differences. Namely:
I think splitting them up into different tables will make it easier to debug them in the future. And it will also make it easier to review these PRs. 😃
Screenshots
Testing