-
Notifications
You must be signed in to change notification settings - Fork 119
feat: Basic context menu #85
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
feat: Basic context menu #85
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Hmmmm. I have mixed feelings. This is definitely a good way to quickly get this working! That said, I'd really like to use native UI where possible, and it is possible to use native platform context menus via Electron... But supporting both is probably helpful - that way we can use Electron's context menu wherever possible, or fall back to a local HTML context menu the rest of the time. The Electron API would be async & imperative (we have to pass & listen for messages) and we can't define it in terms of JSX (though internally the browser version could certainly use JSX to render). At the top level it would have to look something like this: // An event callback, called when a row is right clicked:
function onRowContextMenu(exchange) {
// Calls some kind of global API, or a provider injected via React context etc:
openContextMenu(
{ exchange }, // Some data we can hold & pass back to item click listeners later
[ // The menu to show:
{ type: "item", "id": "toggle-pinned", text: "Toggle Pinned" }
{ type: "separator" }
{ type: "submenu", label: "Copy", items: [
{ type: "item", "id": "copy-decoded-body", text: "Decoded Body" }
] }
],
onContextMenuClick // The callback
);
}
function onContextMenuClick(menuData, itemId) {
// Do something for the clicked item
} On the Electron side, it would show that menu using the native UI, at the current mouse position, and send a message back (calling the callback) if one was clicked. The UI just needs to call the latest registered callback with the id & data if that happens. On the browser side, if Electron isn't available, we'd use react-contextify as here, passing that data to a React component that renders at the current position. If an item gets clicked, it does the same. Maybe we could even replace the ids with just registering a callback on each item... Dunno, I can see arguments either way. What do you think? I'm open to this, but it would need to fit into that framework - as an API independent from specific UI components here (like the view list) where the definition is instead passed through as data, that we can translate into the two formats (HTML vs Electron API calls). That way we can potentially start with just react-contextify, but aim towards using Electron APIs instead where possible in future without a major refactor. |
I don't mind refactoring to a base interface and having the two implementations but what benefits are there to a native context menu? I would assume some minor airspace improvements (although it looks like contextify works around most of the ones one would normally run into). Aside from maintaining two code paths you have the downside that you are not going to be able to customize the context menu to nearly the same extent (at least not without a lot of extra code). Happy to go down that route, just confirming there is some benefit to having a native option as well. |
Yes, native UI is definitely preferred where it's easy to do (like here). There's quite a few users who strongly prefer native components, and in general they do fit in better with user expectations both visually and in UX (e.g. shortcuts, accessibility options) across all different operating systems. And of course it's simpler at runtime - the operating system manages all the UI and details for us, we just need to use the API. In fact, now that I think about it, you'll run into issues here because we already have an automatic context menu that's applied just by the desktop shell, to provide standard OS options when you're using Electron, e.g. it shows 'Select all', 'Copy', 'Paste', and context-specific options like 'Save image as' if you right click any image. This definitely needs to integrate with that, as currently this will probably show both context menus on top of on another in some cases. |
Don't worry I can extend the default. |
ALR well while I interfaced this out and implemented the HTML version I think it was all for nothing. This library not only allows for a pretty straightforward native context menu for electron but it easily can do the extending to preserve the default functionality you speak of https://github.com/sindresorhus/electron-context-menu . The problem is customization based on the item here is the master issue: electron/electron#35632 its possible with a side channel one could do it semi efficiently with some swapping. I would say if much larger projects haven't found a way to do it efficiently it may be worth considering. The context menu here does not need to always be exposed either, that means one could still allow the native context menu (which could even be extended with the project linked) for things like text boxes, or images. |
Yes - we already use this here (that repo has all the Electron parts). I don't think it's a hard problem to integrate the two - the first comment in the issue you linked is indeed exactly what we want to do, and will work just fine: we would listen for context-menu events in the renderer (i.e. in this repo, in the web page), then manually trigger the Electron context events from there with the given menu items when the page has an opinion on what those should be (whenever they're not just the defaults), using I think you don't need to worry about the Electron part here, I'll sort it out separately. If you can refactor this towards an API like the one above (i.e. an API that defines the menu items as a data structure) then it is definitely possible to use that to handle the context menu via Electron later. |
bfe9d9e
to
f3a4525
Compare
OK here is a potential base interface. This is still very much just a draft as I have some example code in it, and am not familiar enough with react to know the best way to do the model/view separation. In theory I think the native one will require a proxy implementation of ContextMenu forwarding things to the external renderer but you probably know more on that one. Interface is pretty basic to implement just needs:
There are 3 menu item types, all can be hidden, all but the separator can be disabled and have display labels:
A few items I ran into that there may be a better way on:
It is having to repeat the EventClickedData I wanted to avoid but this is the only way I could get it to infer properly. Again, the code certainly needs different organization, I could guess better spots but still feel they would be wrong until I get a better idea of the code space and pattern. I have a disable of sample code (for real time control of context menu items) first time you right click pin is enabled, second time disabled, etc. Clearly not something useful here but wanted to document the pattern I was expecting. The copyToClipboard utility function in view-event-list.tsx doesn't go there vs some utility file, but not sure if it is desired or not. Mainly done to work around some of the default chrome restrictions on copying depending on host. |
f3a4525
to
e75e27a
Compare
OK I refactored this to use a strongly typed parameter system and updated my notes above. I like it more, implementers now have an easier time with the data passed). I do have code as well ready for pin favorite / exclude certain headers but it depends on the context menu so I will wait to any version of this gets added then refactor and do it as its own PR. Here is what is working (based off this branch): mitchcapper/httptoolkit-ui@feature_context_right_click_menu...header_include_exclude_sample |
Pass property info to disable/hidden functions as well
84ed629
to
5675eff
Compare
This also moves markdown & the UI store itself into that same model. General idea here is to put anything that doesn't actually use React or define a visual component directly, but does think closely about UX & user-visible interactions, in here as a component/model middleground.
This splits this context menu options completely from the event row UI, separated from the HTML/desktop context triggering & handling, separate from the two actual implementations (one here with react-contexify, one just added in the desktop).
This was broken, because if you haven't opened the request yet (common for context menu items) then the body decoding hasn't started, so the export always includes a "BODY NOT AVAILABLE" warning. If we just wait for decoding like this first, that goes away.
I started digging into this - it's a good start and there was a lot of helpful stuff here, but it didn't quite fit into the wider architecture so it needed a bit of refactoring. I got a bit carried away patching that, and then implementing & integrating a desktop native context menu to test that integration route, and then refactoring various other existing features and integrating them too, to enable all the other context menu buttons, but it's now all totally complete and working! I'll merge this now. That will immediately make this available everywhere using the react-contexify implementation, and then for any future users who install the latest desktop version (coming shortly) it will automatically use the native context menu APIs instead. Ironically, the one thing it doesn't do is to solve the original httptoolkit/httptoolkit#212 issue - on closer reading, that's actually talking about right clicking on the body editor within the request/response, not in the list. That's an issue related to Monoco's own built-in context menu system, not to right clicking on individual rows. I think this will help solve it later, but it's a separately problem for now. This does handle the other cases though, as listed in httptoolkit/httptoolkit#162 and httptoolkit/httptoolkit#69 - it includes all the menu options suggested there. Thanks for contributing! I've been looking at doing this for ages and it's really great to finally get this added 👍 |
started off super simple and honestly half this code is in the wrong place but a small concise example of a very extendable context menu. Works in browser, works in electron. Context menu code is MIT licensed so I believe should be good there.
ClosesRelated to httptoolkit/httptoolkit#212The context menu lib https://github.com/fkhadra/react-contexify supports icons, theming, etc so seems like it should be a good fit. POC implements body copy and pinning for the one standard list view item.
The webkit hack is due to:
vuejs/pinia#675
there may be a better way.
Personally, aside from the items already mentioned in other tickets for context menu items I think things like right clicking and pinning parts of requests/responses (ie a specific header, content length, etc) and then having the request/response cards when collapsed only show the pinned items. Would make it much quicker to potentially look at just the data you want from request to request.
Obviously the myriad of automatic rule adding options (right click on a header add a rule requiring that header, etc).
Anyway I figured get a basic version in first that is acceptable and go from there.