Skip to content
This repository has been archived by the owner on Jul 27, 2018. It is now read-only.

Support Alt-1 ... Alt-9 keyboard shortcuts. #171

Merged
merged 1 commit into from
Mar 4, 2015

Conversation

fabricedesre
Copy link
Contributor

No description provided.


const selectTab = advance(switchSelected,
indexOfSelected,
() => { return index - 1; });
Copy link
Contributor

Choose a reason for hiding this comment

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

_ => index - 1

@paulrouget
Copy link
Contributor

On Mac, we need to use meta, not tab.
Can you rename goToLastTab to selectedLat, maybeGoToTab to maybeSelectIndex and not use start at 0, not 1 (drop index - 1).

@fabricedesre
Copy link
Contributor Author

I fixed the naming and off by one thing.

For the meta/alt, it's the same issue we have with alt + left to go back in history, ie. we need a better way to express os specific bindings. Can we do that in a follow-up?

@@ -67,7 +68,7 @@ define((require, exports, module) => {
'accel right': goForward,
'escape': stop,
'accel r': reload,
'F5': reload
'F5': reload,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: accidental comma? I'm ok with trailing commas but usually avoid them everywhere because of the allocation problems with arrays. Do we have a preference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's not an array, and that makes cleaner diffs when you add items.

fabricedesre added a commit that referenced this pull request Mar 4, 2015
Support Alt-1 ... Alt-9 keyboard shortcuts.
@fabricedesre fabricedesre merged commit 29c96a3 into browserhtml:master Mar 4, 2015
@fabricedesre fabricedesre deleted the keyboard-alt-1-9 branch March 4, 2015 18:55
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.

4 participants