Skip to content

Conversation

@jackkav
Copy link
Contributor

@jackkav jackkav commented Sep 20, 2024

  • persist runner request order
  • fix key value sort order (would previously only let you move up or down one position)
  • remove useListData

future work

  • moving multiple items reverses the order

Ref: INS-4452
Ref #7980

@jackkav jackkav changed the title Persist-runner-sort-order Persist runner request order Sep 20, 2024
@jackkav jackkav self-assigned this Sep 21, 2024
@jackkav jackkav marked this pull request as ready for review September 21, 2024 12:08
@jackkav jackkav requested a review from a team September 23, 2024 08:39
filfreire
filfreire previously approved these changes Sep 23, 2024
@ihexxa
Copy link
Contributor

ihexxa commented Sep 23, 2024

I propose to take care of at least following 2 problems before moving forward:

  • User could change requests after settings (request selection) are persisted.
  • The persisted settings should be saved for different workspaces, as requests don't not exist for another workspace.

And the approach of persisting selection looks good.

@jackkav jackkav force-pushed the persist-runner-sort-order branch from 75bbd79 to 80f318e Compare September 23, 2024 09:20
@jackkav jackkav merged commit a842587 into Kong:develop Sep 23, 2024
@jackkav jackkav deleted the persist-runner-sort-order branch September 23, 2024 09:33
});
const [allKeys, setAllKeys] = useLocalStorage<string[]>(localStorageKey + 'allKeys', { defaultValue: requestRows.map(item => item.id) });
// request was added or removed
if (allKeys.length !== requestRows.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the condition is more complicated, e.g. this check might not work in this case: user adds one new request and deletes one while actually one request has changed.

if (newRequest) {
setAllKeys([...allKeys, newRequest.id]);
} else {
setAllKeys(allKeys.filter(key => requestRows.map(r => r.id).includes(key)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user could move one request to another place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats true, feel free to create a new PR to address this.

@sentry
Copy link

sentry bot commented Sep 30, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Expected fetch controller: :r4h: app:///index.html View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants