Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

added support for pinning/unpinning tabs #124

Merged
merged 2 commits into from
Nov 5, 2017
Merged

Conversation

JiaboHou
Copy link
Contributor

@JiaboHou JiaboHou commented Nov 1, 2017

Added

  • pin active tab (tabs.pin)
  • unpin active tab (tabs.unpin)
  • toggle pin/unpin active tab (tabs.pin.toggle)

default: zp toggles the pinning of the active tab

Demo

https://www.youtube.com/watch?v=zlJskAqSwDI

@ueokande ueokande added this to the 0.5 milestone Nov 1, 2017
@ueokande
Copy link
Owner

ueokande commented Nov 1, 2017

@JiaboHou Thank you for your wonderful changes. I'll merge it on v0.5 release if no conflicts.

README.md Outdated
@@ -34,6 +34,7 @@ The default mappings are as follows:
- <kbd>K</kbd>, <kbd>J</kbd>: select prev or next tab
- <kbd>r</kbd>: reload current tab
- <kbd>R</kbd>: reload current tab without cache
- <kbd>p</kbd>: toggle pin/unpin current tab
Copy link
Owner

Choose a reason for hiding this comment

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

Please use another keys, because p is normally used for paste. Vim Vixen does not support paste URL currently, but I wanna reserve it.

Copy link
Owner

Choose a reason for hiding this comment

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

Vimium uses <A-P>, but Alt keys may conflicts on system bindings.

Copy link
Owner

Choose a reason for hiding this comment

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

While z is a prefix for zoom, I think z is natural prefix for change current state.
How about use zp to toggle pin/unpin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'm cool with that 👍

return browser.tabs.query({ currentWindow: true, active: true })
.then(() => {
let newPinned = pinned;
if (newPinned !== true && newPinned !== false) {
Copy link
Owner

Choose a reason for hiding this comment

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

An interface of this method "toggles when pinned is not boolean" is confusing. Please define another method to toggle pin status, or set pinned on method calling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

features:
 * pin tab
 * unpin tab
 * toggle pin/unpin tab
@JiaboHou JiaboHou force-pushed the pin-tab branch 3 times, most recently from 10f882a to c98ff7a Compare November 5, 2017 23:03
 * change default keybinding for pin/unpin tab from 'p' to 'zp'
 * use toggleTabPinned to toggle the tab pinned state, instead of updateTabPinned
Copy link
Owner

@ueokande ueokande left a comment

Choose a reason for hiding this comment

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

That's great, thank you

@ueokande ueokande merged commit 256820f into ueokande:master Nov 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants