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

Remember panel height when creating it #32

Merged
merged 1 commit into from
Jan 11, 2015
Merged

Remember panel height when creating it #32

merged 1 commit into from
Jan 11, 2015

Conversation

sryze
Copy link
Contributor

@sryze sryze commented Jan 10, 2015

Fixes #31

@noseglid
Copy link
Owner

I didn't get around to give this a whirl yet, but just looking at the code it seems as the build panel will attach regardless of the keepVisible configuration.

I'll check this out in more depth tomorrow. Thanks for your contribution!

@sryze
Copy link
Contributor Author

sryze commented Jan 11, 2015

It will attach, yeah, but then it'll also detach once keepVisible is read from the config if it's false.

It seems like there might be a problem with double attach though (if keepVisible is true). Should I check if the panel has been already created in attach?

@@ -17,6 +17,7 @@ module.exports = (function() {
this.titleLoopIndex = 0;
this.a2h = new Convert();
this.monocle = false;
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.

Is you acutely pointed out, when we call atom.config.observe with the keepVisible argument, visibleFromConfig will be called with the current choice the user have. This attach is not necessary then, because it will be either attach or detach att that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then if we remove this attach this.height will be uninitialized when minimizedHeight gets loaded from the config.

The whole point was that we need to get the height of the panel while it still exists beacuse otherwise the height will be 0. And since the panel might not have been ever made visible prior to reading minimizedHeight we need to explicitly create it somewhere.

Changing the order of observe calls might work too I guess, but this seemed more reliable and I wasn't sure if they are synchronous or not.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah. I see.

You are correct. I think this is the best way too, and it works well. Thanks!

@noseglid
Copy link
Owner

The problem with multiple attach, if it is that the .bottom.tool-panel.panel-bottom element is left (which gives an increasing black bar at the bottom). I found this while testing your fixed and corrected it in 8b182af. (by always destroying it before creating a new one).

noseglid added a commit that referenced this pull request Jan 11, 2015
Remember panel height when creating it
@noseglid noseglid merged commit ae90c5b into noseglid:master Jan 11, 2015
@noseglid
Copy link
Owner

Thanks! I will include this in next release (v0.23.0).

@sryze sryze deleted the height-fix branch January 11, 2015 12:04
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.

Minimized Height option is ignored
2 participants