Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fix for issue #657#658

Closed
ryanstewart wants to merge 2 commits into
adobe:masterfrom
ryanstewart:keybindingswitch
Closed

Fix for issue #657#658
ryanstewart wants to merge 2 commits into
adobe:masterfrom
ryanstewart:keybindingswitch

Conversation

@ryanstewart

Copy link
Copy Markdown
Contributor

Fixed the issue (at least on Mac) about key bindings being out of order compared to other Mac apps.

@tvoliter

Copy link
Copy Markdown
Contributor

Reviewing...

@ryanstewart

Copy link
Copy Markdown
Contributor Author

What's the order on Windows? I don't have a Windows machine handy. @adrocknaphobia ?

Comment thread src/command/KeyMap.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix Ryan. It looks like Windows lists Ctrl first while on Mac Command is last.Can you update your code with the following to handle both platforms?

if (hasCtrl) {
// Windows display Ctrl first, Mac displays Command symbol last
if (brackets.platform === "win") {
keyDescriptor.unshift("Ctrl");
} else {
keyDescriptor.push("Ctrl");
}
}

@tvoliter

Copy link
Copy Markdown
Contributor

done reviewing

@ryanstewart

Copy link
Copy Markdown
Contributor Author

Tweaked as per @tvoliter's code. Haven't tested on windows yet though.

@tvoliter tvoliter mentioned this pull request Apr 17, 2012
@jasonsanjose

Copy link
Copy Markdown
Member

Ty integrated this change in #667. @ryanstewart Please confirm and feel free to close out this pull request without merging.

@tvoliter

Copy link
Copy Markdown
Contributor

Ryan, I copied your code into another pull request I had since I was merging some of your code anyway.

@tvoliter tvoliter closed this Apr 17, 2012
@ryanstewart

Copy link
Copy Markdown
Contributor Author

Sweet, thanks!

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