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

Add sort options for sidebar rows #2738

Merged
merged 14 commits into from
Dec 1, 2020

Conversation

cobwebsonsale
Copy link
Contributor

@cobwebsonsale cobwebsonsale commented Oct 18, 2020

Closes #2594, Closes #2011
Implementation details:

  1. Sending sortHandler function and currentSortOrder from App, all the way down to sidebarFilter
  2. Created a new dropdown on the filter bar to display sort options
  3. Added 7 options to sort the request rows

Assumptions:

  1. For all sort orders, I'm not mixing the request rows inside the requestGroup to those outside
  2. For sorting by HTTP method, since it does not apply to RequestGroups, I move them to the bottom and bring all Requests in a sorted order to the top.
  3. Whenever the sort criteria is the same for two rows, I fall back to metaSortKey to keep the old order preserved. This allows multiple sorts to be applied. For example: Sort by 'name ascending' and then 'folders first' causes RequestGroups to come to top of the list in alphabetical order.
  4. Offered some orders that I thought were useful. Let me know if should edit the list.

I had some questions about the repo/feature:

  1. I wanted to reset sort order to 'CUSTOM' if a document is moved, so that the currentSortOrder is de-selected and can then be re-applied, but we can't access this.state in App._moveDoc(). Is there a reason why this method is defined outside the App class unlike the other methods in the file?
  2. I can't seem to render the new sort-order-dropdown and the create-request-dropdown in the same line (added image below). Can you guys help me with that?
  3. How/where can I add tests?
  4. [not so important] I tried to add icons like fa-sort-alpha-down to the rendering of sort-order display names, but a blank was rendered. Do I have to follow some other steps here?
  5. [not so important] I thought we could save the ordering across different sessions. Should we? If so, what would be the best place to keep that.
  6. [not so important] I wanted to reuse a method _recalculateMetaSortKey inside the old _moveDoc method as well, but the __updateDoc method inside it uses getModel(docToMove.type). I wondered why it was this, and not getModel(doc.type).

Looks like this so far:
sort

- Send handleSortSidebar function from App
- Send currentSortOrder from App
- Create dropdown for sort orders
- Offer different orderings
@nijikokun
Copy link
Contributor

Great stuff, the one thing that I think is obvious is that this pushes the new request to a new line which we definitely don't want, so I'm going to loop in @erickrico to see if he can take a look from a UI/UX perspective

@conema
Copy link
Contributor

conema commented Oct 19, 2020

@cobwebsonsale my two cents: you can try editing the 63th row of packages\insomnia-app\app\ui\css\components\sidebar.less with grid-template-columns: minmax(0, 1fr) auto auto; to make the grid with three colums instead of two. With this edit all the icons should be in the same line, but I feel like that the textarea is too little.

@develohpanda
Copy link
Contributor

develohpanda commented Oct 19, 2020

this pushes the new request to a new line which we definitely don't want, so I'm going to loop in erickrico to see if he can take a look from a UI/UX perspective

@nijikokun there are notes in the original issue (with feedback from Erick) about UX

With this edit all the icons should be in the same line, but I feel like that the textarea is too little.

Are you concerned about the filter text area being too little? How does it look with the default sidebar width? It's potentially okay, otherwise we can maybe reduce the padding on each button.

@develohpanda develohpanda self-requested a review October 19, 2020 21:32
@conema
Copy link
Contributor

conema commented Oct 19, 2020

Are you concerned about the filter text area being too little? How does it look with the default sidebar width? It's potentially okay, otherwise we can maybe reduce the padding on each button.

Yep, It's a little smaller than the one on your screenshot on the issue (I cannot post a screenshot because I resized the sidebar and I don't know how to reset the sizes). Reducing the paddings would be perfect.

@nijikokun
Copy link
Contributor

nijikokun commented Oct 20, 2020

@nijikokun there are notes in the original issue (with feedback from Erick) about UX

Yep, I feel we might need a better solution since it seems like it's cascading into other things (reducing padding, smaller input, responsiveness, etc). Starting to feel crowded and cramped.

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.

Thanks for the PR, and welcome to the project! Nice work. ⭐

I'll answer your questions below.

Assumptions

  1. For all sort orders, I'm not mixing the request rows inside the requestGroup to those outside

Sounds good, we shouldn't alter the parent-child hierarchy 👍

  1. For sorting by HTTP method, since it does not apply to RequestGroups, I move them to the bottom and bring all Requests in a sorted order to the top.

I am assuming this will still sort the requests by method within the request group, though?

  1. Whenever the sort criteria is the same for two rows, I fall back to metaSortKey to keep the old order preserved.

Seems like logical behavior, this is what I would probably expect as a user, but it can be changed with user feedback.

  1. Offered some orders that I thought were useful. Let me know if should edit the list.

They all seem fine to me 👍

Questions

  1. I wanted to reset sort order to 'CUSTOM' if a document is moved, so that the currentSortOrder is de-selected and can then be re-applied, but we can't access this.state in App._moveDoc(). Is there a reason why this method is defined outside the App class unlike the other methods in the file?
  1. [not so important] I thought we could save the ordering across different sessions. Should we? If so, what would be the best place to keep that.

I suggest persisting the sort order in the database under the workspace-meta model, parallel to the sidebarFilter. This way, you can set it to 'CUSTOM' when required, and it will persist correctly between different workspaces.

  1. I can't seem to render the new sort-order-dropdown and the create-request-dropdown in the same line (added image below). Can you guys help me with that?

See the comment above for putting it on the correct line. Spacing, and any positioning changes can be addressed independent of the feature implementation, so try and get it looking nice for now, without expending too much effort on it until confirmation from @erickrico on UX.

  1. How/where can I add tests?

No UI tests exist at the moment, but see my review comments on how to break down the code such that some tests can be added.

  1. [not so important] I tried to add icons like fa-sort-alpha-down to the rendering of sort-order display names, but a blank was rendered. Do I have to follow some other steps here?

It's possible that we are using an older version of FA that doesn't contain this icon. What exactly did you try - <i className="fa fa-sort-alpha-down"/>? We can also add custom icons if necessary (for example).

  1. [not so important] I wanted to reuse a method _recalculateMetaSortKey inside the old _moveDoc method as well, but the __updateDoc method inside it uses getModel(docToMove.type). I wondered why it was this, and not getModel(doc.type).

I'm not sure off the top of my head and it seems a little confusing. Since you've marked it lower priority, I'll defer investigating and answering until the PR has been updated and if this is still a question then.

packages/insomnia-app/app/common/constants.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/common/constants.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/containers/app.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/containers/app.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/containers/app.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/containers/app.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/containers/app.js Outdated Show resolved Hide resolved
- Refactor SortOrder as a Union
- Refactor getSortOrderName as a map
- Add explicit types for props: sortOrder, handleSort
- Change sidebar-sort-dropdown to a function component
- Fix create-dropdown being pushed to the next line
- Add sorting.js helper for _getSortMethod
@cobwebsonsale
Copy link
Contributor Author

Thanks for the PR, and welcome to the project! Nice work. ⭐

Thanks! :)

For sorting by HTTP method, since it does not apply to RequestGroups, I move them to the bottom and bring all Requests in a sorted order to the top.

I am assuming this will still sort the requests by method within the request group, though?

Yep!

[not so important] I tried to add icons like fa-sort-alpha-down to the rendering of sort-order display names, but a blank was rendered. Do I have to follow some other steps here?

It's possible that we are using an older version of FA that doesn't contain this icon. What exactly did you try - <i className="fa fa-sort-alpha-down"/>? We can also add custom icons if necessary (for example).

Yeah, that's what I tried. I think I can keep the order names as text for now, and we can improve the UI in a subsequent PR.

@develohpanda
Copy link
Contributor

Hi @cobwebsonsale! We had a good discussion about this feature, and have a solution now.

I think I get you - what you're saying is to never persist the sort order, it's more of an action taken when selecting a particular ordering, and creating a new request or renaming would not put it in the right location, until you chose to reorder again.

Yep, exactly.

We'll go ahead with this - no persistence of the sort order, it will be an on-click action. With user feedback, we can look to add persistence (and all the related behaviors) in the future. This also removes the need for a checkbox in the sorting dropdown.

I call on @erickrico to share a screenshot of the updated design. There is a higher-level rework we are thinking about for that top section of the sidebar in the future, so no radical changes there, mainly some spacing improvements.

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.

Making progress! I did a quick review 😄

packages/insomnia-app/app/ui/helpers/sorting.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/helpers/sorting.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/helpers/sorting.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/helpers/sorting.js Outdated Show resolved Hide resolved
@cobwebsonsale
Copy link
Contributor Author

Updated:
new_sort

- Rename display names for sort methods to be clearer
- Refactor sort methods to use metaSortKeySort() internally
@cobwebsonsale
Copy link
Contributor Author

I do not have permission to view the logs of the steps that failed in the develop merge commit. Please let me know if I can check them locally somewhere.

@develohpanda
Copy link
Contributor

I do not have permission to view the logs of the steps that failed in the develop merge commit. Please let me know if I can check them locally somewhere.

Netlify is not critical for PRs (only a storybook deployment) so I'll likely ignore it for now. As long as the "Test" GitHub Actions pass, you're all good!

(Netlify is failing to find/install rimraf..., which I've seen with other PRs recently as well)

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.

Great work :) I'm definitely a fan of the unit tests.

As I mentioned in this comment, I'm throwing a spanner in the works with gRPC support. In the last week or so, we've introduced a new model to the sidebar (!), so now the sidebar can show Request, RequestGroup, or GrpcRequest. The good news is that these models are quite similar for your use case, so most of your sorting functions should work out of the box. 😄

The only difference is that GrpcRequest does not have a typical HTTP Method; it's just GRPC, so the HTTP Method sort function will need a small update. I've added some helpers to detect the type of a model in a readable way, so pull from here if you need it!

export function isGrpcRequest(obj: BaseModel): boolean {
return obj.type === grpcRequest.type;
}

If you import this workspace, everything should be set up with an HTTP request and a gRPC request in the sidebar to test with. (Be sure to update with develop first, I only added import support for gRPC on Friday 😅)

packages/insomnia-app/app/common/constants.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/common/sorting.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/common/__tests__/sorting.test.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/components/wrapper-debug.js Outdated Show resolved Hide resolved
- Add sorting for custom http method requests
- Add types SortableModel and SortFunction
- Add tests to increase coverage
@develohpanda develohpanda self-requested a review November 23, 2020 22:05
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.

I'm happy with this now, and SO excited to get it merged! Thank you for the time you put into it and the various iterations. I did some QA locally and it worked great. 🙂

There are a couple of comments I've added, but none of them are blocking. 💯

@develohpanda develohpanda requested a review from DMarby November 24, 2020 01:28
@develohpanda
Copy link
Contributor

Updated:
new_sort

@nijikokun @erickrico please have a look at this final UI. IMO this is complete, any UI changes can be standalone.

- Add comments to httpMethodSort
- Add list of sort orders in constants
- Convert getSortMethod to a map
@cobwebsonsale
Copy link
Contributor Author

@develohpanda, thanks for patiently reviewing this so many times. 🙇‍♂️

@develohpanda
Copy link
Contributor

thanks for patiently reviewing this so many times. 🙇‍♂️

And thank you for working through it! We're on the home-stretch now.

Copy link
Contributor

@nijikokun nijikokun left a comment

Choose a reason for hiding this comment

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

This turned out awesome. Huge props to everyone involved, excited to see it get merged 🤗 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants