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

Add build:toggle-view command #60

Merged
merged 6 commits into from
Apr 30, 2015
Merged

Add build:toggle-view command #60

merged 6 commits into from
Apr 30, 2015

Conversation

sryze
Copy link
Contributor

@sryze sryze commented Apr 26, 2015

This command allows you to show or hide the build view. The default shortcut is cmd-alt-b cmd-alt-v (Mac) or ctrl-alt-b ctrl-alt-v (others).

@sryze
Copy link
Contributor Author

sryze commented Apr 26, 2015

I guess the shortcut should be changed, currently it makes build:trigger wait for the second key press for a little while before building.

@noseglid
Copy link
Owner

I like this!

It's smart to use *-alt-v so it doesnt freeze *-alt-b for a second or so.

I've recently made some changes on how visibility is set (due to #62). Is this compatible with those changes??

if (this.isAttached()) {
this.detach(true);
} else {
this.attach();
Copy link
Owner

Choose a reason for hiding this comment

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

force flag has been added to attach as well in commit cc66db0 this must be passed here as well.

Also, I think this can be written neater by

this.isAttached() ? this.detach(true) : this.attach(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that one looks better, yeah. Though I've mostly seen the ternary operator used when something is actually being returned (i.e. as part of an expression).

@noseglid
Copy link
Owner

I tested it, and it almost works. attach needs the force flag as well.
Have a look at my comments. If you agree, fix them - otherwise argue why you shouldn't :)

Thanks for the effort in implementing this!

@sryze
Copy link
Contributor Author

sryze commented Apr 29, 2015

Should I also rename this to toggle-window? I just noticed that it's called a window everywhere instead of a view (kind of confusing given the class name).

@noseglid
Copy link
Owner

Or build panel (the thing we show is internally called a panel in atom).

Also, other config variables refer to it as build-panel.

@sryze
Copy link
Contributor Author

sryze commented Apr 30, 2015

OK, done.

@noseglid
Copy link
Owner

Excellent. Great job! I will include it in the next version v0.31.0.

noseglid added a commit that referenced this pull request Apr 30, 2015
@noseglid noseglid merged commit de15736 into noseglid:master Apr 30, 2015
@sryze sryze deleted the toggle-view-command branch May 1, 2015 02:16
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.

2 participants