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

MenuBars continued#137

Merged
simonhamp merged 2 commits intomainfrom
feature/menubars-continued
Nov 26, 2024
Merged

MenuBars continued#137
simonhamp merged 2 commits intomainfrom
feature/menubars-continued

Conversation

@simonhamp
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author
simonhamp commented Nov 21, 2024

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
Copy Markdown
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
Copy Markdown
Member Author
simonhamp commented Nov 22, 2024

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
Copy Markdown
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
Copy Markdown
Member Author
simonhamp commented Nov 26, 2024

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
@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