-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
Allow toggling keyboard shortcuts #796
Conversation
b2a8ec4
to
8751728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just a few changes requested, and the status bar item color change doesn't seem to work for me, though the enabling/disabling of key bindings does work.
One more thing I'd like to request is that you add a note about this toggle feature in the Calva docs. I've just moved them into this repo so you can make the changes as part of this PR. See here for details on editing the docs: https://github.com/BetterThanTomorrow/calva/wiki/How-to-Hack-on-Calva#editing-documentation. It's pretty simple. I'm thinking it should be added here: https://calva.io/finding-commands/. Maybe in a new section titled "Toggling Keyboard Shortcuts On/Off". Thanks again for taking the time to contribute. |
Closes BetterThanTomorrow#784 We introduce a `calva.keybindingsEnabled` config option and a `calva.toggleKeybindingsEnabled` command which allows toggling keybindings on and off. The keybindings are updated to only activate when the `calva:keybindingsEnabled` context key is true, in addition to the existing conditions on the keymap.
Previously the Paredit StatusBar's toggleBarItem was being updated throughout multiple setters. Now we refactor the UI update into a single method and remove the internal `_enabled` state which is now unused.
8751728
to
632c346
Compare
This PR is looking great! If @PEZ approves, it can be merged. |
No problem at all. 😃 Like I said early on in the original PR, I am happy to take some time to refine the changes to best fit the needs of all users and your philosophies as the project leads/maintainers. If everyone only submitted the minimum PR to add what they wanted and didn't make changes in response to feedback then either nothing would get merged or the project would become unmaintainable and give a poor UX. Good software comes from collaboration. 😺 I'd like to take a stab at #610 at some point but I'll need to see how tricky this is going to be and how much spare time I have. The contribution process for this PR has been excellent, I feel like my input has been valued and you have thanked me multiple times for taking time to contribute. The docs for contributing (inc the PR template) are really well done and it is obvious that you both care about making Calva a great tool for users and a good experience for contributing to as a project. I'm looking forward to making more improvements in the future. Thanks for all your hard work! ❤️ |
Thanks for the kind words! It seems we are doing some things right, so that's good to hear. We welcome more contributions from you! |
What has Changed?
Closes #784
We introduce a
calva.keybindingsEnabled
config option and acalva.toggleKeybindingsEnabled
command which allows togglingkeybindings on and off. The keybindings are updated to only activate
when the
calva:keybindingsEnabled
context key is true, in addition tothe existing conditions on the keymap.
Also includes minor improvement to Paredit StatusBar code.
Tested in VSCode 1.48.1 on Arch Linux.
My Calva PR Checklist
I have:
dev
branch. (Or have specific reasons to target some other branch.)master
. (Sorry for the nagging.)[Unreleased]
entry inCHANGELOG.md
, linking the issue(s) that the PR is addressing.ci/circleci: build
test. NB: There is a CircleCI bug that makes the Artifacts hard to find. Please see this issue for workarounds.The Calva Team PR Checklist:
Before merging we (at least one of us) have:
dev
branch (unless reasons).Ping @PEZ, @kstehn, @cfehse, @bpringe