Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Add toggle for keymap activation in package settings #610

Merged
merged 4 commits into from
Aug 30, 2015

Conversation

tmunro
Copy link
Contributor

@tmunro tmunro commented Jul 29, 2015

Adds UI support for toggling keymap activation on a package-level. See atom/atom#8130 for more details.

screenshot 2015-07-29 08 58 40

@benogle
Copy link
Contributor

benogle commented Jul 29, 2015

Can you paste in a screenshot for this?

@tmunro
Copy link
Contributor Author

tmunro commented Jul 29, 2015

Absolutely, added screenshot.

@maxbrunsfeld
Copy link
Contributor

Ok, the dependent atom/atom PR is merged, so we can merge this once Atom 1.0.4 has shipped.

This looks good to me UI-wise, though I'd probably tweak the description line (Whether to use the keybindings from this package), or possibly just remove that. Maybe @benogle or @thedaniel would have some ideas for copy on this.

@tmunro Could you add a spec for this new button? You could just assert that the package's name is added/removed from the core.disabledKeymaps config.

Thanks!

@tmunro
Copy link
Contributor Author

tmunro commented Jul 30, 2015

Fixed the broken specs and added that new one. One thing I don't quite understand is why calling my new hasKeymaps method was breaking the specs. I inlined it for now, but it doesn't really make sense to me.

@div class: 'checkbox', =>
@label for: 'toggleKeybindings', =>
@input id: 'toggleKeybindings', type: 'checkbox', outlet: 'keybindingToggle'
@div class: 'setting-title', 'Use Keybindings?'
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about dropping the ? here? I think we tend to avoid punctuation when rendering setting names.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@tmunro
Copy link
Contributor Author

tmunro commented Jul 30, 2015

Removed the ? from the setting name and changed the description to help explain why you might want to use the setting.

@maxbrunsfeld
Copy link
Contributor

OK @tmunro, just a heads up that I've merged atom/atom#8173. Sorry for the delay on that one; I was out on vacation.

@tmunro
Copy link
Contributor Author

tmunro commented Aug 29, 2015

I think this is ready to merge, now that Atom 1.0.8 is out it shouldn't break anything.

@thomasjo
Copy link
Contributor

Not sure I love the the title of the setting (Enable), but someone can always change that later. I'm gonna merge and 🚢 this.

Thanks so much for this @tmunro! ❤️ 💚 💛

thomasjo added a commit that referenced this pull request Aug 30, 2015
Add toggle for keymap activation in package settings
@thomasjo thomasjo merged commit 5877445 into atom:master Aug 30, 2015
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.

6 participants