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 an option to hide/show menu bar. #1090

Closed
wants to merge 4 commits into from
Closed

Add an option to hide/show menu bar. #1090

wants to merge 4 commits into from

Conversation

rstat1
Copy link

@rstat1 rstat1 commented Dec 8, 2015

Adding a menu item to the view menu that allows you hide/show the menu bar. If the menu bar is hidden you toggle it on and off with the "Alt" key.

Fixes #945

Adding a menu item to the view menu that allows you hide/show the menu bar. If the menu bar is hidden you toggle it on and off with the "Alt" key.
@msftclas
Copy link

msftclas commented Dec 8, 2015

Hi @rstat1, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

msftclas commented Dec 8, 2015

@rstat1, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@be5invis
Copy link
Contributor

be5invis commented Dec 8, 2015

Wow that's exactly what I need.

@rstat1
Copy link
Author

rstat1 commented Dec 8, 2015

@be5invis Not bad for someone who's never written Typescript before eh?

@be5invis
Copy link
Contributor

be5invis commented Dec 8, 2015

@rstat1 yeah.

@bpasero
Copy link
Member

bpasero commented Dec 8, 2015

Thanks! But can you not rather use the setAutoHideMenuBar() [1] method instead and avoid having to implement the key listener for Alt? Also, please check if this solution is cross platform, it does not seem to work on Mac and thus should not show up there.

[1] https://github.com/atom/electron/blob/master/docs/api/browser-window.md#winsetautohidemenubarhide

@bpasero bpasero self-assigned this Dec 8, 2015
@bpasero bpasero added this to the Backlog milestone Dec 8, 2015
@rstat1
Copy link
Author

rstat1 commented Dec 8, 2015

@bpasero setAutoHideMenuBar gets me a Property is undefined error because BrowserWindow doesn't have it defined. Is that something I can just go and add, or is the file it's in (atom-browser.d.ts) generated from something?

As far as mac compatibility, it was something I was going to do, and for w/e reason forgot, my mistake.

@bpasero
Copy link
Member

bpasero commented Dec 8, 2015

@rstat1 yes feel free to add it to the d.ts files if missing!

@rstat1
Copy link
Author

rstat1 commented Dec 8, 2015

@bpasero Oh well ok, that simplifies things quite a bit. I'll fix everything up and have a new request ready shortly.

@rstat1
Copy link
Author

rstat1 commented Dec 8, 2015

@bpasero Ok. Done. Menu item disappears on mac, and everything uses the official Electron APIs now instead of my custom stuff.

@bpasero
Copy link
Member

bpasero commented Dec 8, 2015

@rstat1 nice. I wonder if this should not rather be a setting in the user settings configuration? We typically try to put settings into there instead of overloading the menu. I would think of a window.setting that controls this behavior. Otherwise, how do you persist this setting?

@rstat1
Copy link
Author

rstat1 commented Dec 8, 2015

@bpasero There's already items within the "View" menu for toggling other things like the sidebar and full screen mode, so I think it'd make sense to have the menu bar toggle there as well.. With the way settings are currently done in VS Code I don't think that'd be the best place for it.

As far as persisting the toggle across sessions I'd agree that it's something needed.

@bpasero
Copy link
Member

bpasero commented Dec 9, 2015

@rstat1 right, those things are being persisted though (except for fullscreen, but that is for other reasons). We have 2 stories for persisting state: Either in the settings.json which is owned by the user or in local storage which is more for UI state. I think this should probably go into the local storage category as long as there is a menu item to toggle it.

@rstat1
Copy link
Author

rstat1 commented Dec 9, 2015

@bpasero Local storage it is then!

…alStorage whether or not auto hide is active.
@rstat1
Copy link
Author

rstat1 commented Dec 9, 2015

@bpasero Ok, so I added the ability to save whether the menu bar is auto hidden or not. Not sure if I've done everything in the right places.

Also there appears to be merge conflicts that I (being a git/github noob) have no idea how to resolve.

@bpasero
Copy link
Member

bpasero commented Dec 9, 2015

@rstat1 from your branch you can update the pull request easily by merging in the changes from master.

@rstat1
Copy link
Author

rstat1 commented Dec 9, 2015

@bpasero aha! It worked! Thanks :)

@bpasero bpasero removed this from the Backlog milestone Dec 10, 2015
@bpasero
Copy link
Member

bpasero commented Dec 18, 2015

This landed in similar fashion, please see c34d2e7

@bpasero bpasero closed this Dec 18, 2015
@ssaras
Copy link

ssaras commented Mar 16, 2016

For the most part, this also works on Linux Mint 17.1 albeit a bit glitchy.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

Windows: There should be an option to hide the menu bar.
5 participants