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

Docks! #58

Merged
merged 18 commits into from Apr 4, 2019

Conversation

Projects
None yet
5 participants
@50Wliu
Copy link
Member

commented Oct 28, 2017

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

Converts the Keybinding Resolver into a Dock. This fixes some usability issues with the resolver, such as:

  • Variable height causing the keybinding resolver to obscure the entire workspace when a large amount of keybindings are present. Since it's now constrained in a dock, the keybindings will scroll instead.
  • Lack of a close button
  • Inability to view whatever was bound to core:cancel (due to it closing the resolver first)

And adds a nice styling touch to make the header sticky.

Alternate Designs

None.

Benefits

See above.

Possible Drawbacks

I don't foresee any. This should strictly be an upgrade from the existing panel, though I may be missing something. One small thing is that after using the resolver, you might be inclined to close the dock instead of the keybinding resolver. In that case the resolver will still be active and continue to track and display keybindings, resulting in a small performance penalty.

Applicable Issues

Closes #17
Fixes #24
Closes #27
Supersedes and closes #53

/cc @Ben3eeE @UziTech

Most of the boilerplate code was copied from the active-editor-info example package. I don't want to needlessly ping someone, but thanks a lot @matthewwithanm for writing that :). Helped me a lot!

50Wliu added some commits Oct 28, 2017

Do not serialize
This view is unlikely to be something that you'd want to keep open all
the time, even after reloading Atom.  To avoid people forgetting that
they opened the keybinding resolver and then never closing it and
incurring a small performance penalty from the keymap events, we do not
serialize the view so that at least when they close Atom the view will
also close.
@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

Remaining concerns:

  • The keybinding resolver continues to receive keybinding events even when hidden. Is there a way to prevent this behavior to avoid taking a small performance hit?
  • Key Binding or Keybinding? Lots of things use keybinding but there are some places (like the command itself!) that uses key-binding.
  • Panel styles are still being used despite not being a panel anymore
@ungb

This comment has been minimized.

Copy link

commented Jan 25, 2018

I think this is good, I was able to test it now.

Although if you hide the dock(as you mention), it will continue to capture. I think this is acceptable.

I'm all for merging this, but going to /cc @simurai if he has any feedback on the styling.

screenshot:
image

.keystroke {
margin-right: 1em;
.panel-heading {
position: sticky;

This comment has been minimized.

Copy link
@simurai

simurai Jan 26, 2018

Member

Ohh.. wow. Didn't know that works now. We could use that in other places. Like root folders in the tree-view or package titles in the settings.

This comment has been minimized.

Copy link
@50Wliu

50Wliu Jan 26, 2018

Author Member

😍

@simurai

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Yay for no more jumping.. 🎉

Added some small restyling:

Before After
screen shot 2018-01-26 at 1 45 17 pm screen shot 2018-01-26 at 1 39 36 pm

The header background could be changed to look seamless to the tab, but that's something themes have to look into.

The keybinding resolver continues to receive keybinding events even when hidden. Is there a way to prevent this behavior to avoid taking a small performance hit?

Do Docks have some sort of "only run if dock item is visible" option? Maybe ::isVisible()? So that it can be paused while the dock item is hidden.

@ungb

This comment has been minimized.

Copy link

commented Jan 26, 2018

LGTM! 🚢

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

I'll see if I can wire something up with onDidChangeVisible.

@UziTech UziTech referenced this pull request Apr 11, 2018

Closed

add close button #53

@bdowling

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Did this PR slide to the side? Sounds like it's an improvement...

@bdowling

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

fwiw 👍 on this one, I merged it with my PR #62 and it is much more pleasant to use in that mode without it jumping around and covering the editor..

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

I've been quite busy. Intend to return to this one soon.

@bdowling

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

fwiw, my merge is here, https://github.com/bdowling/keybinding-resolver/tree/add_copy_keybinding_with_pr58 -- it's been working well for me, other than the minor conflicts, not sure what else this PR needs to get accepted, but I like it. Thanks!

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Ok, so the issue is isVisible is for the whole dock, and the Resolver doesn't necessarily have to be in focus for isVisible to return true.

50Wliu added some commits Oct 27, 2018

Revert "Do not serialize"
This reverts commit 757a05d.

50Wliu added some commits Oct 27, 2018

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2018

Think I fixed all outstanding concerns! Just in time for the one-year anniversary of this PR :)

New event specs are a little on the weak side but those can always be tweaked later.

@nathansobo nathansobo merged commit 2e200f8 into master Apr 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the wl-dock branch Apr 4, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Thanks for this. Sorry it took so long.

@nathansobo nathansobo self-assigned this Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.