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

Feature/context menu helper #140

Merged
merged 20 commits into from
Dec 1, 2024

Conversation

gwleuverink
Copy link
Contributor

@gwleuverink gwleuverink commented Nov 24, 2024

This PR adds a context menu helper on the window.Native object:

<button
    type="button"
    onclick="Native.contextMenu([
        {
            label: 'Foo',
            click: () => alert('Baz!')
        },
        {
            label: 'Bar',
        },
    ])"
>

This will render a context menu in-place. You can pass anything the Electron Menu template API accepts.

Consideration

It might be possible to do this with the PHP api. Do you think we should add that for pairity?

It appears some commits came along. Not sure why. Can you squash them or should I submit a new PR?

@simonhamp
Copy link
Member

I like this. I'm not sure what we can get from a PHP version (except maybe a Livewire convenience?)

I think we should be cautious about simply surfacing Electron's API tho as we're not in control of that, so if it changes and breaks we just have to pass the break on

@gwleuverink
Copy link
Contributor Author

gwleuverink commented Nov 24, 2024

A php version would mostly be for consistency.

The structure is similar but slightly differs in features. For example if you want to open a external browser using the Native menu helper you'd still have to:

A) hit a endpoint from a menu item click handler that calls the Shell facade
B) export shell.openExternal in your bundle manually

Unless you are open to exposing shell.openExternal on the Native helper too.

@simonhamp
Copy link
Member

We have to be careful about what we expose to the web view as this could be abused by third-party URLs loaded in it, especially if we don't create specific protections

specifically tho, my previous was about just expecting Electron's classes means that we end up with apps that are tightly coupled to Electron's implementation

This isn't a huge concern right now, but later it may be nice to have an abstraction here that lets us absorb any breaking changes and also handle multiple "backends" (ie not just electron)

@gwleuverink
Copy link
Contributor Author

gwleuverink commented Nov 24, 2024

100% agree with you.

Oh now I see, I made an error in my example. Since this uses the Menu template api we can just pass this an array of object literals. I was burning the midnight oil again.

No need for the MenuItem object at all. I've updated the example in the description.

@simonhamp
Copy link
Member

simonhamp commented Nov 25, 2024

No need for the MenuItem object at all. I've updated the example in the description.

Cool cool. But this still has to map to the syntax that buildFromTemplate accepts... which means there's two different ways to handle this. And this still results in the app having a tight coupling to Electron.

Plus we've already handled this on the PHP side.

There's nothing wrong with this, but I wonder if we should encourage something like this instead?

<script>
    const myMenu = @json($menu);
</script>

<button
    type="button"
    onclick="Native.contextMenu(myMenu)"
>

Then in the controller or Livewire component that renders this view:

public function render()
{
    $menu = Menu::make(...);
    return view('button', ['menu' => $menu]);
}

Based on the new syntax that will come from #139.

The structure of the serialised Menu object could conform to what buildFromTemplate accepts/expects... in fact I think it already does.

@gwleuverink
Copy link
Contributor Author

That is really cool. Didn't realize the menu serializes in that way.

Maybe this needs some more thought. If we can do this while enforcing our established Menu api with 100% parity that would be incredible.

How do you suggest we proceed?

@simonhamp
Copy link
Member

Didn't realize the menu serializes in that way.

I'm not 100% certain it does, would need to check. If it doesn't, let's make sure it does!

Once it does, we can just recommend this as best practice in the docs.

Eventually we'll need to do more, but for now this is fine.

@simonhamp
Copy link
Member

@gwleuverink sorry for taking so long to pick this up. AMenu object is now JsonSerializable, which means you can use the @json Blade directive to turn it into JSON as I described above.

@simonhamp simonhamp merged commit 0e1cb65 into NativePHP:main Dec 1, 2024
6 of 10 checks passed
@gwleuverink
Copy link
Contributor Author

Cool! I didn't realize you were waiting on this. Otherwise I would have put in the time earlier 😅 got sidetracked on some side projects.

Is it automatically compatible with links and such? That's really interesting. I should dive how this serializes under the hood

@simonhamp
Copy link
Member

I wasn't 😊 it was waiting on me

Is it automatically compatible with links and such?

I'm not 100% sure what you mean. But it will all come out in the wash

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

Successfully merging this pull request may close these issues.

2 participants