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

Support for pinned requests #1471

Merged
merged 35 commits into from May 7, 2019

Conversation

Projects
None yet
3 participants
@develohpanda
Copy link
Contributor

commented Apr 27, 2019

Closes #1344. Closes #1109.

Following feature set is not as merged given changes from discussions in this PR.

Features

  • Only allows pinning of top level request and request-group
  • Prevent drag/drop of pinned items
    • Dragging nested items into/out of a pinned request-group is possible
    • Manipulation of nested items remains unchanged
  • Independent scrolling for pinned and unpinned items
    • Pinned items always remain at the top
  • Pinned item list + separator shown when at least one pinned item exists
  • Pinned item list height constrained via fit-content(20%)
    • Auto height up to 20%, after which scroll bar will show
    • 20% will allow for 6 items to be pinned before overflow scrollbar shows
  • Pin/Unpin action items added to action drop-downs
  • fa-thumb-tack shown on pinned items

pinned state is stored in request-meta and request-group-meta

Screenshots

No pinned items (current behavior):
image

Pinned folder + request, with unpinned items:
image

Six pinned items:
image

Seven pinned items:
image

Open tag on collapsed, pinned folder to allow dnd of nested items:
image

Action dropdowns:
image
image
image
image

No pin action on nested items:
image

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

I did a once over of the code and it looks great. Nice work!

One high-level thing though is that I think people will want to be able to pin any request, not just the top-level ones. I'm not sure pinning folders is useful, although maybe you can point out some use-cases that I haven't considered.

Overall great work on this. I'm super excited to get this one merged 💯

develohpanda added some commits Apr 27, 2019

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Thank you - I appreciate it! 😄

I'm not sure pinning folders is useful

I see pinned items as those being truly global, which would really only be authentication requests. If all requests are pinned, then none of them are pinned so I agree with your sentiment. 😄 Only example I can think of is with larger solutions with multiple APIs and 3rd party integrations where there may be logical groupings for pinned items.

Your call - given it is probably a niche use-case I could remove the ability to pin folders and add if a request comes up.

people will want to be able to pin any request, not just the top-level ones

  • Enabled the pin action item in nested items
  • Updated parentId to the active workspace id inside the pin handle method
  • I didn't update this behavior for request-groups given the discussion above. Will update accordingly depending on the outcome. :)
@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

Yep that sounds good. Pinned requests for now and folders if people want those later.

Just a little background. The original use case for pins that I remember is basically treating them as tabs (like in a text editor). So you might pin just the few requests you're working with at the moment.

Sent with GitHawk

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Makes sense. Done deal, PR has been updated!

@develohpanda develohpanda changed the title Support for pinned requests and folders Support for pinned requests Apr 28, 2019

@rickychandra

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@develohpanda Hi, just a suggestion. Would it be better if the pin icon is at the left side? Putting it at the right side will take up the dropdown's arrow, and will be moved aside when the request row is hovered over, which IMHO is a bit awkward 😄.

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Would it be better if the pin icon is at the left side?

Thanks for the suggestion @rickychandra! I have a few reasons why I decided against an icon on the left which I'll outline below, but my ears are open to anything I may have missed. 😄 (apologies in advance for the long message!)

  1. Padding inconsistency. If I move it to the left had side, it shifts the folder icon/method tag + text to the right, but only for pinned items. This alignment is inconsistent with unpinned items, and reading at a glance becomes tricky (similar to reading a balance sheet where the decimal point is not aligned with every other line). This is a hindrance and will slow down navigation.

  2. Unnecessary cluttering. Differentiating request from folder (ie. method tag vs folder icon), and one row from another (ie. name) is more important than differentiating a pinned row from an unpinned row. A pinned row will always be at the top regardless, so adding unnecessary cluttering to the first two items (tag/folder + name) only gives more for the user to ingest. This is also a hindrance that will slow down navigation.

  3. Cost/benefit.
    Pin on left
    Cost: styling inconsistencies + unnecessary cluttering that are always visible
    Benefit: no movement of pin when hovering over a row
    Pin on right
    Cost: movement of pin only on hover to make way for dropdown arrow
    Benefit: styling consistency, no UI hindrances introduced for primary tasks (navigating to and identifying a row).

The pin is only supplementary info necessary for certain rows, and is kept out of the way. The drop-down arrow is similar, in that it is supplementary info only for hovered rows, and is kept out of the way (or hidden) when not needed.

image image

@rickychandra

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

The pin is only supplementary info necessary for certain rows

Ah I see. I thought the pin is also clickable, so it would be easier to click if it doesn't move on hovered.

I agree with point 1 and 2, putting the pin on the left can make it hard to skim the items, especially when the pinned status is no longer that important (all pinned items are at the top and are separated with a line with the rest of the items).

Good job anyway! 😄

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Thank you! Appreciate the suggestion 🥂

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

After testing out your branch locally, I have a few usability questions/suggestions:

  1. I expected pinning a request keep showing the request in its original position (I couldn't think any apps that support pinning besides GitHub but this is how GitHub issue pinning behaves there)
  2. I expected the pin icon on the right to unpin the request. Having it move out of the way on hover felt jarring. I think it would either feel better if the pin icon disappeared on hover or that the dropdown caret appeared to the left of the pin icon instead of to the right. Also, if we decide to show both icons, clicking the pin icon should unpin it too.
  3. It would be super useful to have a keyboard shortcut to toggle the pin state of a request. Let me know if you need help with this one.

And a bug: When pinning a request that's in a folder, then unpinning, the request is no longer inside the folder.

Let me know if your thoughts on these or if you have any questions 😸

@gschier
Copy link
Collaborator

left a comment

Really nice work on this. Just a few fairly minor comments 👍

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I expected pinning a request keep showing the request in its original position

Do you mean for the pinned request to show in two places - nested section and its original position? 👍

I think I won't be able to get to these until the weekend. Thank you for the feedback!

@gschier

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

@develohpanda yes, show in both places 😄

Don't worry, take your time!

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@gschier two questions:

  • How should the filter behave with pinned requests? IMO pinned items should not be affected by filters and should always be visible.

  • Given pinned items are to show in both places, should the pin icon also show in both places (or only in the pinned list at the top)?

    When showing items in the pinned list at the moment, it is referencing the same object (as I am not creating a new one). Thus selecting, renaming a row in the pinned list does the same to the request in its original location (and vice-versa). (You can observe this when testing). Therefore I am inclined to to show the pin icon in both places too.

@develohpanda
Copy link
Contributor Author

left a comment

@gschier I have updated the PR with your feedback and added a few comments. Let me know what you think!

pinned: Array<Child>,
all: Array<Child>,
};

This comment has been minimized.

Copy link
@develohpanda

develohpanda May 4, 2019

Author Contributor

Is there a more appropriate place to put this type definition?

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

I'm okay with it here. It makes sense to have it defined where it's used.

this.props.activeRequest,
!(metas[0] && metas[0].pinned),
);
},

This comment has been minimized.

Copy link
@develohpanda

develohpanda May 4, 2019

Author Contributor

Not a big fan of this solution, but the other solution would be to put the pinned on Request rather than RequestMeta, but more changes will need to go into app.js to support it. 🤔

Are there any other options that I am missing?

This comment has been minimized.

Copy link
@develohpanda

develohpanda May 4, 2019

Author Contributor

Just thinking, whatever solution is decided here could also be used to show a pin icon/reordering for pinned requests in the quick switcher.

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

We'll need to keep pinned on RequestMeta. The "meta" models are meant to store information that isn't related to actually sending the request and also that should not be synced or shared.

Note. One thing that would help here is using the getOrCreateByParentId method on request-meta.js.

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

Also, you'll need to add a check to make sure activeRequest is not null

This comment has been minimized.

Copy link
@develohpanda

develohpanda May 4, 2019

Author Contributor

Note. One thing that would help here is using the getOrCreateByParentId method on request-meta.js.

_handleSetRequestPinned internally calls App._updateRequestMetaByParentId, which is the same one used by the other handlers and supports getOrCreate. If it is a new request (with no meta) then this handler also creates meta.

static async _updateRequestMetaByParentId(requestId, patch) {
const requestMeta = await models.requestMeta.getByParentId(requestId);
if (requestMeta) {
return models.requestMeta.update(requestMeta, patch);
} else {
const newPatch = Object.assign({ parentId: requestId }, patch);
return models.requestMeta.create(newPatch);
}
}

This comment has been minimized.

Copy link
@gschier

gschier May 5, 2019

Collaborator

Ya I saw that. I thought your first comment was about the complexity of finding the request meta in the entities list.

I was suggesting using getOrCreate instead of having to write the manual filter on the entities list

};

if (child.pinned) {
pinnedChildren.push(child);

This comment has been minimized.

Copy link
@develohpanda

develohpanda May 4, 2019

Author Contributor

I did this for efficiency. Extracting all pinned requests (ie. nested requests) in sidebar-children would require an extra unnecessary traversal of childrenTree.

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

I think it actually fits well here within the context of "sidebar children" since the only place it's used is in the sidebar. If/when pinned requests are needed elsewhere, this would make less sense. For now, it's good though.

@gschier
Copy link
Collaborator

left a comment

Just a few comments

pinned: Array<Child>,
all: Array<Child>,
};

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

I'm okay with it here. It makes sense to have it defined where it's used.

this.props.activeRequest,
!(metas[0] && metas[0].pinned),
);
},

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

We'll need to keep pinned on RequestMeta. The "meta" models are meant to store information that isn't related to actually sending the request and also that should not be synced or shared.

Note. One thing that would help here is using the getOrCreateByParentId method on request-meta.js.

this.props.activeRequest,
!(metas[0] && metas[0].pinned),
);
},

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

Also, you'll need to add a check to make sure activeRequest is not null

};

if (child.pinned) {
pinnedChildren.push(child);

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

I think it actually fits well here within the context of "sidebar children" since the only place it's used is in the sidebar. If/when pinned requests are needed elsewhere, this would make less sense. For now, it's good though.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2019

How should the filter behave with pinned requests? IMO pinned items should not be affected by filters and should always be visible.

I'll leave this one up to you. I think both ways make sense. We'll leave it to future feedback to tell us if we made the wrong choice

Given pinned items are to show in both places, should the pin icon also show in both places (or only in the pinned list at the top)?

I think in both places makes sense 👍

@@ -288,6 +290,11 @@ const defaultRegistry: HotKeyRegistry = {
keyComb(true, false, false, false, keyboardKeys.d.keyCode),
),

[hotKeyRefs.REQUEST_TOGGLE_PIN.id]: keyBinds(
keyComb(false, false, false, true, keyboardKeys.p.keyCode),

This comment has been minimized.

Copy link
@gschier

gschier May 4, 2019

Collaborator

Mac key comb should probably be false, false, true, true to map to Cmd+Shift+P because Cmd+P already maps to the request switcher. Windows/Linux one is fine though with what you have (Ctrl+Shift+P)

@develohpanda

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Ready again!

@gschier
Copy link
Collaborator

left a comment

Just one small thing!

if (!this.props.activeRequest) {
return;
}

const metas = Object.values(this.props.entities.requestMetas).filter(

This comment has been minimized.

Copy link
@gschier

gschier May 5, 2019

Collaborator

Should be using .find() instead of .filter()

This comment has been minimized.

Copy link
@develohpanda

develohpanda May 5, 2019

Author Contributor

TIL. Will update this evening!

@gschier

gschier approved these changes May 7, 2019

Copy link
Collaborator

left a comment

Looks good now! Thanks so much for your hard work on this one 😄 💯

@gschier gschier merged commit c48daae into getinsomnia:develop May 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@develohpanda develohpanda deleted the develohpanda:feature/pinned-request branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.