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

Desktop: Resolves #1752: New: Option to automatically hide the menu bar #9603

Closed
wants to merge 8 commits into from

Conversation

tinyoverflow
Copy link

This PR adds an option to automatically hide the menu bar on desktop. It can still be shown with the alt key. There were two other related posts to this:

I noticed on caveat: When toggling the menu bar, the webview or it's content doesn't resize automatically. This can be fixed by resizing the window, though. I have absolutely no experience with Electron and therefore couldn't solve this. Any help regarding this issue is appreciated.

image

Copy link
Contributor

github-actions bot commented Dec 25, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@tinyoverflow
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Dec 25, 2023
value: false,
type: SettingItemType.Bool,
section: 'application',
public: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might make sense to hide this setting on MacOS (as the documentation marks that method as Windows and Linux only).

To see how this is done, we do something similar for the autoUpdate setting on Linux:

autoUpdateEnabled: { value: true, type: SettingItemType.Bool, storage: SettingStorage.File, isGlobal: true, section: 'application', public: platform !== 'linux', appTypes: [AppType.Desktop], label: () => _('Automatically check for updates') },

(The autoUpdate setting probably shouldn't be hidden on Linux, so the above example may cause a bug.)

Copy link
Author

Choose a reason for hiding this comment

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

Good point, thanks!

I've added a condition similar to the one on autoUpdateEnabled, as you've mentioned.

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Dec 28, 2023

Thank you for working on this!

Be aware that we have a script that generates release notes from pull request titles. As such, I suggest changing Desktop: Implement PR #1752: Option to automatically hide the menu bar to Desktop: Resolves #1752: New: Option to automatically hide the menu bar. It may also make sense to check whether there's an issue (rather than a pull request) for this already (maybe #3894?).

@tinyoverflow tinyoverflow changed the title Desktop: Implement PR #1752: Option to automatically hide the menu bar Desktop: Resolves #1752: New: Option to automatically hide the menu bar Dec 29, 2023
@tinyoverflow
Copy link
Author

tinyoverflow commented Dec 29, 2023

Thanks for your feedback!
I've updated the PR title accordingly to your suggestion 👌

#3894 targets a different issue/feature request, imho. Though it is related to this PR, I'd prefer to create a seperate pull request for the feature requested in that issue.

I've also changed a few more bits in this PR:
The updateMenuBar method is now only called when the current device is not a Mac. The method itself also received a guard clause to return early in case it's called anyway somewhere else in the future.

In addition to that, I've changed the window instantiation to hide the menu bar by default on startup. This is to prevent the menu bar from being visible for a short amount of time when the window loads before it get's hidden again. If hiding is disabled, it will be shown together with when the app gets rendered.

@laurent22
Copy link
Owner

As discussed in the original PR it should be a View menu item, like in VSCode for example:

image

@personalizedrefrigerator personalizedrefrigerator added the desktop All desktop platforms label Jan 7, 2024
@laurent22
Copy link
Owner

@tinyoverflow, thank you for the pull request but as mentioned above it would be better if it's a menu item. Making it a menu item appears to be more in line with other commonly used applications, and it also makes it possible to associate a keyboard shortcut with the action.

Please let us know if you need any assistance to complete the pull request.

@laurent22
Copy link
Owner

Closing for now but feel free to let us know if you'd like to give it another try. Sorry we couldn't merge the PR as it is.

@laurent22 laurent22 closed this Jan 10, 2024
@tinyoverflow
Copy link
Author

tinyoverflow commented Jan 10, 2024

@laurent22 Thanks. I unfortunately do not have that much time available at the moment. I'll give it another shot at the weekend :)

@LightTreasure
Copy link
Contributor

Hi @tinyoverflow, I've implemented @laurent22's suggestion of making this a menu item, making my changes off of your fork+branch. Would you like me to create a PR on your fork, or should I make a new PR in this repo?

@tinyoverflow
Copy link
Author

Hello @LightTreasure,

unfortunately I couldn't find any time digging deeper into this.
I'm fine with both ways you've suggested. If you want to do a PR to my repo, so this PR gets updated, I'll make sure to credit you in this PR.

Thanks for taking this!

@LightTreasure
Copy link
Contributor

@tinyoverflow I'll make a PR on you repo! I built on top of your work :)

@LightTreasure
Copy link
Contributor

@tinyoverflow Here's the PR: tinyoverflow#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants