Menu improvements #423

simonhamp · 2024-11-20T03:09:39Z

Heads up, this introduces some significant breaking changes!

We've been discussing internally for a while how the Menu API sucks a little. There are a few quirks, it doesn't really expose the stuff you probably want, and it just feels awkward, it doesn't feel very "Laravel".

You new() up a menu and then you have to register() it, which is what sets it as the application menu. It's not immediately obvious (imo) that you can omit register() and pass the resulting Menu to other things like Windows, the MenuBar and the Dock.
Defining items that should be in a list and can be nested inside each other as part of fluent chain feels really strange. You have to 'break' the chain (conceptually) in places that make it feel awkward and inconsistent, and the code ends up looking jarring rather than familiar.
The fluent chain is nice, but because we're immediately adding sparse instances to the chained Menu we lose access to the underlying MenuItem classes and so can't make deeper customisations easily.
It's currently impossible to change the application Menu once it's been defined.

So this is my attempt to improve on all of these issues by:

Introducing a new Menu facade which wraps a new MenuBuilder class
This menu builder let's you access the underlying MenuItem objects more easily and build your menus with a more expressive DX. More on this in a moment...
More MenuItem types that map to standard OS functions (called Roles)
Making Link menu items default to changing the currently-focused window's URL onclick without any extra wiring.
If you want to open in the user's browser (the current default), you will be able to chain the new ->openInBrowser() method to Menu::link()
Making submenus (which are just Menu objects, which are a MenuItem too) definable in a more consistent, expressive way
Adding a new default() helper to create the default menu even more easily
Enhance event data coming back from menu item clicks, such as forwarding other data like combo keys
Allow adding of an ID to MenuItem instances - useful in handling generic MenuItemClicked events on the Laravel side
Allow the app menu to be completely recreated at any point in the app lifecycle 💪🏼

Here's what this looks like:

Create the application menu

Before

use Native\Laravel\Menu\Menu;

Menu::new()
    ->appMenu()
    ->editMenu()
    ->register();

After

use Native\Laravel\Facades\Menu;

Menu::create(
    Menu::app(),
    Menu::edit(),
);

Menu::create builds the menu and registers it all in one shot.

Create a menu for use elsewhere (e.g. as a context menu, Dock menu, or MenuBar menu)

Before

use Native\Laravel\Menu\Menu;

Menu::new()
    ->link('https://nativephp.com', 'Documentation');

After

use Native\Laravel\Facades\Menu;

Menu::make(
    Menu::link('https://nativephp.com', 'Documentation')
        ->openInBrowser(),
);

Define the default menu

Before

use Native\Laravel\Menu\Menu;

Menu::new()
    ->appMenu()
    ->editMenu()
    ->viewMenu()
    ->windowMenu()
    ->register();

After

use Native\Laravel\Facades\Menu;

Menu::default();

Menu::default() allows you to easily return your menu to the default that would be provided if you didn't register a menu at all.

Define a custom menu

Before

use Native\Laravel\Menu\Menu;

Menu::new()
    ->appMenu()
    ->submenu(
        'My Submenu',
        Menu::new()
            ->link('https://nativephp.com', 'Documentation')
    )
    ->register();

After

use Native\Laravel\Facades\Menu;

Menu::create(
    Menu::app(),
    Menu::make(
        Menu::link('https://nativephp.com')
            ->label('Documentation')
            ->openInBrowser(),
        // or: Menu::link('https://nativephp.com', 'Documentation'),
    )->label('My Submenu'),
);

Menu::make() returns a Native\Laravel\Menu\Menu instance.

Many of the other helpers return a MenuItem subclass, from which you can chain calls to their other methods.

This gives you far more control over your menus. Building your menus has never been more straightforward.

Define a blank menu

Before

use Native\Laravel\Menu\Menu;

Menu::new()
    ->register();

After

use Native\Laravel\Facades\Menu;

Menu::create();

Fold it all into Link instead

SRWieZ

It's quite a change in syntax, but I believe this PR improves it.

Some docblocks are incorrect or "incomplete"
I prefer using "hotkey" instead of "accelerator."
I got a bug with my custom event on a menu item throwing an exception because of a missing named argument on my constructor. I think we should either ask devs to extend MenuItemClicked with their event or better create a contract that they should implement with the following constructor: public function __construct(public array $item, public array $combo = [])

Overall it's great!

Also we should add a note in the documentation stating that we cannot change the name of the first menu in macOS. Those two examples does not work, always shows with config('app.name'):

Menu::create(Menu::edit())

Menu::create(Menu::make()->label('My label'))

SRWieZ · 2024-11-22T19:25:52Z

src/Facades/Menu.php

+use Native\Laravel\Menu\Items\Separator;
+
+/**
+ * @method static \Native\Laravel\Menu\Menu make(MenuItem ...$items)


This references the wrong MenuItem, it should reference Native\Laravel\Menu\Items\MenuItem

SRWieZ · 2024-11-22T19:26:42Z

src/Facades/Menu.php

+ * @method static \Native\Laravel\Menu\Menu make(MenuItem ...$items)
+ * @method static Checkbox checkbox(string $label, bool $checked = false, ?string $hotkey = null)
+ * @method static Label label(string $label)
+ * @method static Link link(string $url, string $label = null, ?string $hotkey = null)


It's named $hotkey here and $accelerator in the class.
I prefer $hotkey

SRWieZ · 2024-11-22T19:28:51Z

src/Menu/Items/MenuItem.php

@@ -3,11 +3,15 @@
 namespace Native\Laravel\Menu\Items;

 use Native\Laravel\Contracts\MenuItem as MenuItemContract;
+use Native\Laravel\Facades\Menu as MenuFacade;
+use Native\Laravel\Menu\Menu;

 abstract class MenuItem implements MenuItemContract


Can we create an alias to accelerator() as hotkey() ?

SRWieZ · 2024-11-22T19:31:39Z

src/Events/Menu/MenuItemClicked.php

@@ -12,7 +12,7 @@ class MenuItemClicked implements ShouldBroadcastNow
 {
    use Dispatchable, InteractsWithSockets, SerializesModels;

-    public function __construct(public array $item) {}
+    public function __construct(public array $item, public array $combo = []) {}


Can we add some PHP Doc here to represent:

array:2 [▼ "item" => array:2 [▼ "label" => "Settings" "checked" => false ] "combo" => array:5 [▼ "shiftKey" => false "ctrlKey" => false "altKey" => false "metaKey" => false "triggeredByAccelerator" => false ] ]

gwleuverink · 2024-11-22T22:18:43Z

Honestly this works great! I've tinkered with it a bit now.

One thing I might be missing. Context menu's were mentioned. Is it possible to trigger a menu in-place on a button click? This doesn't work right now and could be a cool addition.

I've got a alpine magic I use for this. I'm happy to extract that to the Native helper if you want.

Right now it looks like this:

<button
    type="button"
    x-on:click="
        $contextMenu([
            new MenuItem({
                label: 'Foo',
            }),
            new MenuItem({
                label: 'Bar',
            }),
        ])
    "
>

It does involve adding the MenuItem class to the window scope. But that can be namespaced under the Native helper if needed.

I haven't tested this, but it might be feasible to add this to the php api instead. The code is pretty slim.

gwleuverink · 2024-11-22T22:22:14Z

Extracted to the Native helper I imagine something like this:

<button
    type="button"
    onclick="Native.contextMenu([
        new MenuItem({
            label: 'Foo',
        }),
        new MenuItem({
            label: 'Bar',
        }),
    ])"
>

Example of what that looks like:

What's cool about this is that you can invoke Livewire actions asynchronously from the MenuItem click callback:

new MenuItem({
    label: 'Restart run',
    click: async () => $wire.restartRun({{ $run->remote_id }}),
}),

gwleuverink · 2024-11-22T22:51:36Z

It does involve adding the MenuItem class to the window scope. But that can be namespaced under the Native helper if needed.

Turns out using Menu.buildFromTemplate I can construct this menu from a array of plain objects. So no need to expose more than needed to the window

simonhamp · 2024-11-23T18:20:15Z

@gwleuverink that context menu stuff looks super cool! Fancy wrapping whatever you've needed to modify there into a new PR?

gwleuverink · 2024-11-24T03:27:24Z

@gwleuverink that context menu stuff looks super cool! Fancy wrapping whatever you've needed to modify there into a new PR?

Done! NativePHP/electron#140

simonhamp and others added 16 commits November 20, 2024 00:06

Extract MenuBuilder

a3a8936

Add more Electron MenuItem roles

bff79d7

Allow MenuItems to have submenus

9d407ce

enabled/disabled

97bc40b

Add GoToUrl convenience item

0e2429f

Fix styling

428819b

Add help and hide roles

e066697

Receive combo key data

4ad23a6

Add id's to menu items

c1874ee

Support radio items

37f0040

Style

23c3363

Remove custom event menu item type

7c1cca5

Support custom event firing on all menu items

f857938

Fix label

1c8931e

Type hints and consistency

c6cc7e0

Fix styling

a180a16

simonhamp requested a review from mpociot November 20, 2024 03:16

simonhamp added 2 commits November 20, 2024 09:44

Get rid of the yucky GoTo* stuff

a6a6f30

Fold it all into Link instead

Fix test

3825635

SRWieZ suggested changes Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menu improvements #423

Menu improvements #423

simonhamp commented Nov 20, 2024 •

edited

Loading

SRWieZ left a comment •

edited

Loading

SRWieZ Nov 22, 2024

SRWieZ Nov 22, 2024

SRWieZ Nov 22, 2024

SRWieZ Nov 22, 2024

gwleuverink commented Nov 22, 2024 •

edited

Loading

gwleuverink commented Nov 22, 2024 •

edited

Loading

gwleuverink commented Nov 22, 2024 •

edited

Loading

simonhamp commented Nov 23, 2024

gwleuverink commented Nov 24, 2024

Menu improvements #423

Are you sure you want to change the base?

Menu improvements #423

Conversation

simonhamp commented Nov 20, 2024 • edited Loading

Create the application menu

Before

After

Create a menu for use elsewhere (e.g. as a context menu, Dock menu, or MenuBar menu)

Before

After

Define the default menu

Before

After

Define a custom menu

Before

After

Define a blank menu

Before

After

SRWieZ left a comment • edited Loading

Choose a reason for hiding this comment

SRWieZ Nov 22, 2024

Choose a reason for hiding this comment

SRWieZ Nov 22, 2024

Choose a reason for hiding this comment

SRWieZ Nov 22, 2024

Choose a reason for hiding this comment

SRWieZ Nov 22, 2024

Choose a reason for hiding this comment

gwleuverink commented Nov 22, 2024 • edited Loading

gwleuverink commented Nov 22, 2024 • edited Loading

gwleuverink commented Nov 22, 2024 • edited Loading

simonhamp commented Nov 23, 2024

gwleuverink commented Nov 24, 2024

simonhamp commented Nov 20, 2024 •

edited

Loading

SRWieZ left a comment •

edited

Loading

gwleuverink commented Nov 22, 2024 •

edited

Loading

gwleuverink commented Nov 22, 2024 •

edited

Loading

gwleuverink commented Nov 22, 2024 •

edited

Loading