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

Feat header include exclude context menu #87

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mitchcapper
Copy link
Contributor

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.

@pimterry
Copy link
Member

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:

  • A visible menu button, shown at the side of each header, or
  • Buttons shown inside the expandable header area instead, or
  • A separate "pinned data" section above the headers, like the 'watch' section in a debugger, where you can monitor interesting header/url/body/etc values, all in one place, or
  • Part of a larger request diffing feature, to compare specific headers between requests directly, or...

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! 😄

@mitchcapper
Copy link
Contributor Author

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.
The addition of context menus I think complements this very well as it allows for easy testing without a lot of ui redesign or new components.

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:
image

A separate "pinned data" section above the headers, like the 'watch' section in a debugger, where you can monitor interesting header/url/body/etc values, all in one place

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.

Part of a larger request diffing feature, to compare specific headers between requests directly, or...

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:)

@mitchcapper mitchcapper force-pushed the feat_header_include_exclude_context_menu branch from 923e894 to d57ca72 Compare September 26, 2023 23:16
@mitchcapper
Copy link
Contributor Author

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).

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pimterry pimterry force-pushed the main branch 2 times, most recently from e0e350c to 1d713c8 Compare May 7, 2024 20:22
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.

None yet

3 participants