-
Notifications
You must be signed in to change notification settings - Fork 25
DEVPROD-1977: Update tests table to use LeafyGreen #2213
Conversation
1 failed and 5 flaky tests on run #15285 ↗︎
Details:
Review all test suite changes for PR #2213 ↗︎ |
Should the title be tests table? |
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.
idw block you since you've been waiting on this for a while! Let me know if you need another look!
|
||
const queryVariables = getQueryVariables(search, task.id); | ||
const { limitNum, pageNum, sort } = queryVariables; | ||
const [queryParams, setQueryParams] = useQueryParams(); | ||
const { execution, limitNum, pageNum, sort } = queryVariables; | ||
const cat = sort?.[0]?.sortBy; |
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 is not really part of your changes, but could we rename this cat
variable to be more clear what it represents? :o
header: () => ( | ||
<>{task.versionMetadata.isPatch ? "Base" : "Previous"} Status</> |
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.
Can this be:
header: () =>
`${task.versionMetadata.isPatch ? "Base" : "Previous"} Status`
table.resetColumnFilters(); | ||
table.resetSorting(); | ||
setQueryParams({ | ||
...queryParams, |
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.
Do we want to pass queryParams
here if we are just going to clear them? If there's something specific we want to preserve, it might make sense to use updateQueryParams
instead
...queryParams, |
}; | ||
} | ||
updateQueryParams(queryParams); | ||
const clearQueryParams = () => { |
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 noticed some flaky behavior on this button where sometimes the filters didn't actually clear (this is on beta, with only original PR changes applied). I couldn't really tell why this happens though... 🤔
Screen.Recording.2024-01-16.at.11.59.56.AM.mov
updateQueryParams({ | ||
[RequiredQueryParams.Category]: TestSortCategory.Status, | ||
[RequiredQueryParams.Sort]: SortDirection.Asc, | ||
[TableQueryParams.SortBy]: TestSortCategory.Status, | ||
[TableQueryParams.SortDir]: SortDirection.Asc, |
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 also possible to set execution here using the execution from the task
prop, not sure if that's a good option though, just wanted to mention it
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.
Since all of the task tabs require an execution I think it's preferable to include it in a higher level shared component!
DEVPROD-1977
Description
Screenshots
Testing