-
Notifications
You must be signed in to change notification settings - Fork 108
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 header include exclude context menu #87
base: main
Are you sure you want to change the base?
Feat header include exclude context menu #87
Conversation
Very interesting! Thanks for sharing this. I can definitely see how this could be useful, but I try to avoid shipping new features like this to the core product without broader user feedback first, both to ensure there's demand and to get more context on exactly how it should work and what the use cases are. For example, there are arguments that in different scenarios this should instead be:
Deciding what to ship is hard without broader feedback and an understanding of how people would use this! I effectively have to maintain added features forever, and implementing the 'wrong' thing makes it much harder to do the 'right' thing later, so some caution is required. That said, I'm open to following through with this in future, I just want to understand the wider demand first. I'll keep this PR open for now, and I've created an issue for the idea that people can vote on & share feedback over here: httptoolkit/httptoolkit#460 Of course, in the meantime if it's useful to you, you can keep using it yourself on your fork anyway! 😄 |
Makes sense. It might be worth considering adding an experimental/labs options page where you can set some persistent preferences. This could allow for more rapid prototyping and testing of features and allow you to collect actual usage telemetry to decide if it is worthwhile to invest further resources into it. Experimental features would not be a guaranteed long term ship item (avoiding the forever commitment) but things httptoolkit was considering. Another option would be the massive work of adding plugin support per #56. It would likely not be hard to add a context menu hook (so users could hook context menus on specific items to add actions) but I would guess the UI side of things is a goot bit trickier. While it is just HTML/DOM given the fact it is react a plugins ability to randomly mess with a UI component seems tough compared to other platforms. Such features could be off by default, and only enabled by advanced users. As for adding additional direct UI components for this feature it can limit how many things you can do if each has their own icon. That can be partially mitigated by making a much more configurable UI page where multiple features can be turned on or off by users, so only things that are applicable can show up for them. I do think there is merit for context menus to be more discoverable though if their usage continues to grow. Permanent UI like you mentioned elsewhere of triple dots next to a header or here "A visible menu button" works for some of the UI things but if the context menu is a list item specific action it makes less sense (and adding ... next to every item supporting it may be a bit much). One option is the VS style approach where there is a solid context menu for nearly every window/row/line but when you hover over an item you can also get a small icon in the gutter showing there are actions that can be taken ie:
That is a very cool idea. It could be combined with this feature by adding a "Favorites" or similar list to the IncludeExcludeList allowing for both top pinned items and still context specific pinning. I would go a step further if you add a top-pinned data section and have order up/down arrows next to each item. Ordering itself isn't as simple if items are maintained using multiple includeexcludelist as currently designed. I would guess the general way this feature would work is either asking child components for any favorite items or them pushing favorites up to the top pinned area. If the top pinned area kept a secondary array with not just the key name but also prefixing that with the source component (ie request_headers_cookies) storing that one additional array would give you persistent options there.
ahhhhhhhhhhhhhhhh that does sound like a massive task to implement and require more complicated user actions to make it happen. IE I may just be looking to see what cookies have changed from one request to another but if I have to multi-select requests then right click and go to diff then find the item I care about for simple things far easier if I can just visually diff items as I arrow key through requests. Anyway, yes will happily keep my forks for things I find useful just like to contribute back what may be useful to others:) |
923e894
to
d57ca72
Compare
Updated this to the latest api calls, it could use further revisions to match the constructor/function passing style of the other context menus now but figured not put that work in until the feature gets the green light. Also updated FilterArrayAgainstList / SortArrayAgainstList the list handler to take a transformer function to get the value to use as the key rather than always assuming the object itself (similar to the _sort calls now). |
|
e0e350c
to
1d713c8
Compare
I tried to follow the structuring / view / model format you used for the other context menu, so should at least be closer to correct:)
There is no auto-refresh of the header list when you pin/exclude something maybe the re-render should be called.
Excluded headers could be in an already collapsed CollapsibleSection rather than totally not visible, although i think this works just fine.
I in-vision CollapsibleCard could be changed to be a tri-state component instead. Rather than just expanded or collapsed it could also only show the currently pinned headers, sort of a compact form.
I added load/save handlers to the more generalized IncludeExcludeList class. I think there is persistent data with httptoolkit (probably just premium only) and I haven't fooled with that at all so I didn't actually hook them up to any serializer to save them to w/e persistent store.
IncludeExcludeList is somewhat generalized in the list data type but hardcoded to the
IEList
enum for the lists themselves. It could be added as a second generic parameter instead to expand beyond the limited 3 mentioned. Granted it could just add more added, as nothing requires a implementer to use all the lists it supports.I like the more dynamic implementation of the context menu code, allowed me to do the on the fly generation for currently excluded headers so you can clear just one at a time easily.