fix(*): ktabledata adoption [KHCP-13280] #1660
Conversation
| @@ -390,6 +392,7 @@ const modalContent = computed(() => { | |||
| const toggleEnableStatus = (row: EntityRow) => { | |||
| enablementModalVisible.value = true | |||
| switchEnablementTarget.value = row | |||
| delete switchEnablementTarget.value.selected | |||
There was a problem hiding this comment.
I think we need to discuss the API design for this. There may be similar cases throughout the codebase that we've overlooked. Ideally, IMO, there should be a selectedRowKeys: string[] prop for <KTableView> so that we don't have to write into userland row data and then fix potential issues in an ad-hoc manner. And before that we should have a proper row key first as we have discussed.
/cc @adamdehaven @portikM
There was a problem hiding this comment.
A little context: I added this line because there was an issue when toggling the enabled status of a gateway service:
selected is included in the request payload, resulting in a 400 error. This is caused by how the bulk actions functionality is implemented in KTableView: it adds selected to the original data.
We feel instead of deleting the selected field every time we see an issue, it may be better to re-think the implementation of bulk actions as @Justineo suggested.
There was a problem hiding this comment.
I agree, this aligns with some of the concerns I brought up on the original KTableView PR on reserved keys
There was a problem hiding this comment.
We can change the selected key to something more specific like bulkActionsSelected so it doesn't collide with plugins enabled status. Would that be sufficient?
There was a problem hiding this comment.
We can change the selected key to something more specific like
bulkActionsSelectedso it doesn't collide with pluginsenabledstatus. Would that be sufficient?
To be honest I think this is the issue with “reserved keys” - the component shouldn’t have a requirement limiting the keys utilized in the data object in the first place. Yes, something named this specifically will likely prevent a collision; however, I’m still not convinced this is the desired pattern.
There was a problem hiding this comment.
I suggest we implement the mini refactor we discussed above as a fix-forward patch.
There was a problem hiding this comment.
- There's an existing issue in KTableView with using indexes as keys that presents an issue when a row is deleted
- The bulk actions key and selected keys need to be refactored into separate props based on this PR discussion
There's no reason to release a new Kongponents component that utilizes KTableView under the hood with these substantial issues/changes in flight
There was a problem hiding this comment.
Why can't I attempt to fix the issue like I suggested? Once we confirm the fix is sufficient, the changes in flight will be the minor under-the-hood implementation change
There was a problem hiding this comment.
Update: we decided to implement the refactor in KTableView and proceed with merging KTableData and deprecating KTable afterwards to avoid having to test it twice. I will add a sction in the original KTableView component brief discussion the need for new rowKey prop. After the refactor is implemented we shouldn't need the fix deleting an unexpected property from the row object.
There was a problem hiding this comment.
The refactor has been implemented, this is ready a final review.
| :disable-sorting="disableSorting" | ||
| :empty-state-options="emptyStateOptions" | ||
| enable-entity-actions | ||
| :error-message="errorMessage" | ||
| :fetcher="fetcher" | ||
| :fetcher-cache-key="fetcherCacheKey" | ||
| :hide-toolbar="hideTableToolbar" |
There was a problem hiding this comment.
question: what criteria should be used to determine when the toolbar should be hidden?
There was a problem hiding this comment.
The toolbar is hidden when fetcher returns no records and there is no search query or filters. Something like a "true" empty state. In that case none of the toolbar items (search, column visibility menu) are actionable.
Summary
Jira: https://konghq.atlassian.net/browse/KHCP-13280
Changes:
InitialLoadandNoRecordsfetcher states in useFetcher composableResources