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

Added a clippy to the list to copyKeybindings #62

Merged
merged 2 commits into from Sep 11, 2018

Conversation

Projects
None yet
3 participants
@bdowling
Contributor

bdowling commented Aug 18, 2018

Similar to the keybindings list in settings-view, the clippy copies the binding
for easier editing / keymap customizations

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This is a simple change to add a 'clippy' icon to allow the keybindings to be copied. This is the same code from the settings-view on keybindings.

Edit by @rsese to add the screenshots

keybinding-screenshot
keybinding-screenshot-atom-dark

Alternate Designs

Benefits

This makes it so much easier to customize your keybindings by finding where the conflicts exist and overriding them in your local keymap.

Possible Drawbacks

People spend all day customizing their keymaps.

Applicable Issues

Looks like can close #34 which was written some time ago with similar intent, but this ain't coffee anymore ;)

Closes #30
Closes #34

@bdowling bdowling changed the title from Added a clippy to the list to copyKeybings to Added a clippy to the list to copyKeybindings Aug 18, 2018

Added a clippy to the list to copyKeybindings
Similar to the keybindings list in settings-view, clicking the clipboard icon copies
the key binding for easier editing / keymap customizations

@bdowling bdowling referenced this pull request Aug 20, 2018

Open

Docks! #58

@rsese

This comment has been minimized.

Show comment
Hide comment
@rsese

rsese Aug 20, 2018

Member

Thanks for contributing! Can you add a screenshot that shows where the icon will be added?

Member

rsese commented Aug 20, 2018

Thanks for contributing! Can you add a screenshot that shows where the icon will be added?

@bdowling

This comment has been minimized.

Show comment
Hide comment
@bdowling

bdowling Aug 20, 2018

Contributor

Can we try to get #58 merged too and I'll update the docs/images to show both UI changes? ;)

Contributor

bdowling commented Aug 20, 2018

Can we try to get #58 merged too and I'll update the docs/images to show both UI changes? ;)

@bdowling

This comment has been minimized.

Show comment
Hide comment
@bdowling

bdowling Aug 22, 2018

Contributor

keybinding-screenshot
keybinding-screenshot-atom-dark

Contributor

bdowling commented Aug 22, 2018

keybinding-screenshot
keybinding-screenshot-atom-dark

@rsese

This comment has been minimized.

Show comment
Hide comment
@rsese

rsese Aug 22, 2018

Member

Can we try to get #58 merged too and I'll update the docs/images to show both UI changes? ;)

Sorry missed your comment here - it looks like you already dropped a comment in #58 and Winston will drop an update if he can, it seems like there was something else he wanted to look at based on his last comment. I can go ahead and ask the team to take a look at this PR on its own for now.

And thanks for dropping those screenshots 👍 I added them to the issue body.

Member

rsese commented Aug 22, 2018

Can we try to get #58 merged too and I'll update the docs/images to show both UI changes? ;)

Sorry missed your comment here - it looks like you already dropped a comment in #58 and Winston will drop an update if he can, it seems like there was something else he wanted to look at based on his last comment. I can go ahead and ask the team to take a look at this PR on its own for now.

And thanks for dropping those screenshots 👍 I added them to the issue body.

@bdowling

This comment has been minimized.

Show comment
Hide comment
@bdowling

bdowling Aug 23, 2018

Contributor

fwiw, I've tried to resubmit a couple times this pass to clear the flaky CI error, #60, but it does not appear to be working for Travis. I don't believe I have anything that affects that here, but I've been wrong before.

Contributor

bdowling commented Aug 23, 2018

fwiw, I've tried to resubmit a couple times this pass to clear the flaky CI error, #60, but it does not appear to be working for Travis. I don't believe I have anything that affects that here, but I've been wrong before.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Sep 11, 2018

Member

Thanks a lot for your contribution @bdowling!

Member

daviwil commented Sep 11, 2018

Thanks a lot for your contribution @bdowling!

@daviwil daviwil merged commit 9718b15 into atom:master Sep 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment