Skip to content
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

Merged
merged 2 commits into from
Sep 22, 2020

Conversation

wcerfgba
Copy link
Contributor

@wcerfgba wcerfgba commented Sep 22, 2020

What has Changed?

Closes #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.

Also includes minor improvement to Paredit StatusBar code.

Tested in VSCode 1.48.1 on Arch Linux.

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. NB: There is a CircleCI bug that makes the Artifacts hard to find. Please see this issue for workarounds.
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • Created the issue I am fixing/addressing, if it was not present.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

@wcerfgba wcerfgba mentioned this pull request Sep 22, 2020
21 tasks
@wcerfgba wcerfgba marked this pull request as ready for review September 22, 2020 08:48
@bpringe bpringe changed the base branch from master to dev September 22, 2020 16:50
Copy link
Member

@bpringe bpringe left a 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.

src/paredit/statusbar.ts Outdated Show resolved Hide resolved
src/paredit/statusbar.ts Outdated Show resolved Hide resolved
src/paredit/statusbar.ts Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@bpringe
Copy link
Member

bpringe commented Sep 22, 2020

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.

John Preston added 2 commits September 22, 2020 18:57
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.
@wcerfgba wcerfgba changed the title Toggle keybindings Allow toggling keyboard shortcuts Sep 22, 2020
@wcerfgba wcerfgba requested a review from bpringe September 22, 2020 18:00
@bpringe
Copy link
Member

bpringe commented Sep 22, 2020

This PR is looking great! If @PEZ approves, it can be merged.

@PEZ
Copy link
Collaborator

PEZ commented Sep 22, 2020

This is awesome. I realise it can feel like you are torn In all different directions @wcerfgba , but I think Calva benefits from us all engaging and providing perspectives. I vote for merging now.

Many thanks for the help @wcerfgba , I hope you find more things you want to improve.

@wcerfgba
Copy link
Contributor Author

I realise it can feel like you are torn In all different directions @wcerfgba , but I think Calva benefits from us all engaging and providing perspectives.

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! ❤️

@bpringe
Copy link
Member

bpringe commented Sep 22, 2020

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!

@bpringe bpringe merged commit 5d7a723 into BetterThanTomorrow:dev Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow toggling keyboard shortcuts
3 participants