8000 MenuBars continued by simonhamp · Pull Request #137 · NativePHP/electron · GitHub
[go: up one dir, main page]
Skip to content

MenuBars continued #137

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 26, 2024
Merged

MenuBars continued #137

merged 2 commits into from
Nov 26, 2024

Conversation

simonhamp
Copy link
Member
@simonhamp simonhamp commented Nov 19, 2024 •
edited
Loading
  • Allow future calls to the menu bar to recreate it
  • Support more events in a standard way
  • Hide the popover when right-clicking to show the context menu

@simonhamp simonhamp mentioned this pull request Nov 19, 2024
Merged
@SRWieZ
Copy link
Member
SRWieZ commented Nov 21, 2024

Tested while working on 2 different MenuBar app

  • one with a web route
  • the other with only a context menu

No regression to report.

{
  "nativephp/electron": "dev-feature/menubars-continued",
  "nativephp/laravel": "dev-feature/menubars-continued"
}

@SRWieZ
Copy link
Member
SRWieZ commented Nov 21, 2024
  • Hide the popover when right-clicking to show the context menu

I'm not sure I understand this. I didn't see any changes. I tried several combinations of right-clicking and left-clicking, and all seem fine both before and after.

@simonhamp
Copy link
Member Author

and all seem fine both before and after.

I was getting the popover remaining open and the context menu appearing on top of it... not sure how. But I explicitly added a line to hide the popover window on right-click and it fixed it for me

@SRWieZ
Copy link
Member
SRWieZ commented Nov 22, 2024 •
edited
Loading

Okay, I see it now. I understand.

I didn't think it was a bug.

I tried to test other apps, but I don't have many apps that have both. The only one that I use is Jetbrains Toolbox, and their context menu appears on top of the popover.

To be fair, not many apps will use both the context menu and the popover.
Often, the context menu has just the Quit option.

I gave it a thought for a bit and I clicked a thousand times on every icons of my navbar:

  • As long as it opens on top, it doesn't bother me.
  • In the old version I had an app with a "refresh" option in my context menu bar that refreshes my popover, so.. the old way can be a feature
  • I don't think people should use both popover and context menu at the same time but they do whatever they want
  • Every time I click an icon in my menu bar, the app opens while the other one closes.
    • This does not apply to the NativePHP popover UNLESS I click on another NativePHP app or JetBrains (popover) menubar, which I suspect to be Electron too.
    • JetBrains Toolbox also closes automatically if you click another menubar, but with a delay. I wonder if it's the result of an event listener. We should probably find an event to replicate this behavior.

So. After an hour clicking around my final take is:

  • If you prefer that the popover closes before opening the context menu: go for it! I personally don't care.
  • We should go a little step further and close the menu bar when focus is lost and alwaysOnTop:false. It's ugly when you have two different app on top of each other.

I found someone having the same issue there: max-mapper/menubar#155
Which links to this bug where they says its fixed (but only for contextMenu I believe): electron/electron#8689 (comment)
Same link contain, this workaround: electron/electron#8689 (comment)

And as why I wasn't seeing any changes, the dist/ version of the js file is not committed so I wasn't really testing the new javascript part.

For future me: after changing branch with composer.json, don't forget to build the plugin:

composer update -W
cd vendor/nativephp/electron/resources/js
npm install && npm run plugin:build

An artisan command would be nice 😊

@simonhamp
Copy link
Member Author

there's a lot to unpack here 😅

As long as it opens on top, it doesn't bother me.

Yeh I get that. It seemed jarring to me and I hadn't consciously seen that behaviour in other apps. I can confirm I see it in Jetbrains Toolbox and I don't like it there either 😆

I think you countered yourself later with "It's ugly when you have two different app on top of each other." - I know this isn't technically another app, but it's a different visual/action context.

That said...

the old way can be a feature

Agreed. Let's see what people ask for. I don't mind being wrong, but my personal preference is not to have this happen.

I don't think people should use both popover and context menu at the same

Maybe there's some Apple style guide about it, but I think it's really useful. Notion has it, for example

Every time I click an icon in my menu bar, the app opens while the other one closes

Not sure what you mean by this exactly. Can you elaborate?

We should go a little step further and close the menu bar when focus is lost and alwaysOnTop:false

I've not experienced an issue where the menu bar app stays open when i click elsewhere... it always closes for me when it loses focus... except for when clicking on a different context menu-only tray app after I open my app, then I get this:
Screenshot 2024-11-22 at 11 41 59

Which I also dislike. Jetbrains Toolbox doesn't have this problem. So I will try to fix this.

@SRWieZ
Copy link
Member
SRWieZ commented Nov 22, 2024

Yeah sorry for the long comment 😄
You did get all of it thought .

I can confirm I see it in Jetbrains Toolbox and I don't like it there either 😆

So go for it! I believe it's better to move quickly at this stage. Being opinionated isn't a bad thing. I don't think people will miss that "feature." Personally, I will update my app to place the refresh button on the popover instead of the context menu.

... except for when clicking on a different context menu-only tray app after I open my app, then I get this:
[image]
Which I also dislike. Jetbrains Toolbox doesn't have this problem. So I will try to fix this.

That's what I mentioned in my second part regarding the issues I linked. I believe we need to use the workaround I linked to make it function like JetBrains. Just remember to first check if alwaysOnTop == false.

@simonhamp
Copy link
Member Author

That's what I mentioned in my second part regarding the issues I linked. I believe we need to use the workaround I linked to make it function like JetBrains. Just remember to first check if alwaysOnTop == false.

I'm going to do this in a future PR

@simonhamp simonhamp merged commit baae218 into main Nov 26, 2024
3 of 10 checks passed
@simonhamp simonhamp deleted the feature/menubars-continued branch November 26, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simonhamp @SRWieZ
0