-
Notifications
You must be signed in to change notification settings - Fork 25
DEVPROD-3049: Remove LeafyGreen column utils #2181
Conversation
1 flaky test on run #14671 ↗︎
Details:
cypress/integration/version/unscheduled_patch/configure_patch.ts • 1 flaky test
Review all test suite changes for PR #2181 ↗︎ |
retries: { | ||
runMode: 3, | ||
openMode: 0, | ||
}, |
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!
@@ -31,7 +31,7 @@ describe("Task Duration Tab", () => { | |||
cy.dataCy("leafygreen-table-row").should("have.length", 3); | |||
cy.location("search").should( | |||
"include", | |||
"duration=DESC&page=0&statuses=running-umbrella,started,dispatched" | |||
"duration=DESC&page=0&statuses=running-umbrella%2Cstarted%2Cdispatched" |
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 indicates to me that the query params aren't being properly uriencoded or that we are double encoding them.
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.
Well isn't the URL now being encoded, and it wasn't before? ,
is a reserved character in the URI specification which is why commas included our query param value should be encoded (as per these docs). query-string's .stringify()
has encode = true as default, which is what ultimately applies the encoding.
}, | ||
[updateQueryParams] | ||
); | ||
const updateFilters = (filterState: ColumnFiltersState) => { |
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 think at some point here we aren't properly transforming array values for filters which causes them to not be uriencoded correctly.
const [filters, setFilters] = useState<ColumnFiltersState>(initialFilters); | ||
const [sorting, setSorting] = useState<SortingState>(initialSort); |
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 seems like we are just duplicating the state that's already represented in the URL.
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 should be able to just rely on the urlstate.
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.
Yeah, react-table's handling of sort/filter is kinda weird... I went with this approach because it allows us to use the onColumnFilterChange
/onSortingChange
callbacks; using non "manual" filtering (to use react-table terminology) means that those functions are never called. But I changed approaches to have a useEffect
hook update the query params instead.
Let me know what you think! If you like it I'll make a similar change to HostsTable.
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.
Looks great!
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.
💯
DEVPROD-3049
Description
Testing