Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting same sidebar item sorting for the Edit tag request dropdown #2777

Merged
merged 9 commits into from Dec 16, 2020

Conversation

antoine29
Copy link
Contributor

This PR is adding a method to get the same item sorting as is in the request left sideBar, for the tag requests dropdown. For the item sorting I'm using the same sorting criteria found in this selector code.
And as you can see if the sidebar item sorting is rearranged, the change will be reflected in the tag requests dropdown too.
tagReqsSorted
This would close #2745

@@ -103,6 +103,23 @@ class TagEditor extends React.PureComponent<Props, State> {
);
}

_sortRequests(requests: Array<any>, parentId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use types to specify that this is an array of Requests and RequestGroups. Like suggested here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

types added :D

Copy link
Contributor

Choose a reason for hiding this comment

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

😆 we picked up on the same thing

#2777 (comment)

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Overall looks good, I've got some minor suggestions. 🙂

Sorry about the delay in getting to this, I've been really bogged down for a while!

@@ -103,6 +103,23 @@ class TagEditor extends React.PureComponent<Props, State> {
);
}

_sortRequests(requests: Array<models.request.type | models.requestGroup.type>, parentId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can import the type directly and use it. models.request.type is actually just a string 😆

import type { Request } from '../../../models/request';
import type { RequestGroup } from '../../../models/requestGroup';
...
_sortRequests(models: Array<Request | RequestGroup>, parentId: string) { 

Also, it seems that using models instead of requests seems more appropriate here for the parameter, since an item in the array can actually be a Request or a RequestGroup. By extension, when iterating through the array it would also change to models.filter(model => ...)

if (a.metaSortKey === b.metaSortKey) return a._id > b._id ? -1 : 1;
else return a.metaSortKey < b.metaSortKey ? -1 : 1;
})
.map(request => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit picking, but this should likely be a .forEach rather than .map since we don't use the result for anything.

Comment on lines 115 to 116
if (request.type === models.request.type) sortedReqs.push(request);
if (request.type === models.requestGroup.type)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've created some helpers in packages/insomnia-app/app/models/helpers/is-model.js to make these a little less verbose. It would then change to:

if (isRequest(model)) { }
if (isRequestGroup(model)) { }

Comment on lines 110 to 113
.sort((a, b) => {
if (a.metaSortKey === b.metaSortKey) return a._id > b._id ? -1 : 1;
else return a.metaSortKey < b.metaSortKey ? -1 : 1;
})
Copy link
Contributor

Choose a reason for hiding this comment

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

#2738 is almost complete and it creates a metaSortKeySort helper, linked below. Ideally that is used here, so once that PR is merged in we can update this.

https://github.com/cobwebsonsale/insomnia/blob/bca044f74d8bf437b6775ac5a9cf4c9f5f52ba3d/packages/insomnia-app/app/common/sorting.js#L92-L98

Then, this code would just become:

Suggested change
.sort((a, b) => {
if (a.metaSortKey === b.metaSortKey) return a._id > b._id ? -1 : 1;
else return a.metaSortKey < b.metaSortKey ? -1 : 1;
})
.sort(metaSortKeySort)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, l lost sight of this PR hehe. Anyways I added the required changes :D

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice! Looks awesome now 👏🏽 Thanks for working through it

@develohpanda develohpanda merged commit 171dae5 into Kong:develop Dec 16, 2020
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.

Sort request list in dropdown of "Response -> Body attribute" tag editor
5 participants