Adding commands+shortcuts for keyboard nav of tabs in dock panel.#1647
Conversation
|
@sccolbert for review Another question: right now I don't do anything if a user tries to select the next tab and they are at the end of the active panel. Is there a reasonable way of iterating through the panels in the dock panel? |
|
I think I see the logic for going between bars in the I have the |
|
We usually call toArray if were are going to do any indexing on an iterator. |
ellisonbg
force-pushed
the
dock-panel-shortcuts
branch
from
7ee4a1d to
69abd61
Compare
|
Woohoo!! I have fun with this one. It is all working. This makes it really easy to navigate the entire dock panel using |
|
Squeaked it in for 0.16! I'll test/review this now. |
| private _previousTabBar(): TabBar { | ||
| let current = this._currentTabBar(); | ||
| if (current) { | ||
| let bars = toArray<TabBar>(this._dockPanel.tabBars()); |
There was a problem hiding this comment.
the <TabBar> type is not needed. toArray will automatically infer the generic type based on the argument.
| private _nextTabBar(): TabBar { | ||
| let current = this._currentTabBar(); | ||
| if (current) { | ||
| let bars = toArray<TabBar>(this._dockPanel.tabBars()); |
| }, | ||
| { | ||
| command: ApplicationCommandIDs.activatePreviousTab, | ||
| selector: 'body', |
There was a problem hiding this comment.
do these shortcuts not conflict with anything else in the app?
There was a problem hiding this comment.
Not that I know of...
|
Made a few comments. Looks reasonable. |
| if (current) { | ||
| let ci = current.currentIndex; | ||
| if (ci !== -1) { | ||
| if (ci < (current.titles.length-1)) { |
There was a problem hiding this comment.
spaces around the binary operator
There was a problem hiding this comment.
While we're being nit-picky, you don't need the extra parens either. The - operator has a higher precedence than logical operators: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence
There was a problem hiding this comment.
I will remove the extra parens. Can you clarify what you mean about the spaces around the binary operator?
There was a problem hiding this comment.
current.titles.length-1 -> current.titles.length - 1
There was a problem hiding this comment.
Nevermind, I understand...
| if (ci < (current.titles.length-1)) { | ||
| current.currentIndex += 1; | ||
| current.currentTitle.owner.activate(); | ||
| } else if (ci === (current.titles.length-1)) { |
| let prevBar = this._previousTabBar(); | ||
| if (prevBar) { | ||
| let len = prevBar.titles.length; | ||
| prevBar.currentIndex = len-1; |
| let ci = bars.indexOf(current); | ||
| let prevBar: TabBar = null; | ||
| if (ci > 0) { | ||
| prevBar = bars[ci-1]; |
There was a problem hiding this comment.
idem (and two lines down)
| let len = bars.length; | ||
| let ci = bars.indexOf(current); | ||
| let nextBar: TabBar = null; | ||
| if (ci < (len-1)) { |
|
This is really elegant, well done! |
|
K, should be good, thanks for the review! |
|
🎉 |
lock
Bot
added
the
status:resolved-locked
3 participants
Fixes #1637
This works, but I have a few questions:
ApplicationShell?