-
Notifications
You must be signed in to change notification settings - Fork 116
Feat: Initial multi-select support for events #95
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
base: main
Are you sure you want to change the base?
Feat: Initial multi-select support for events #95
Conversation
crap of course I selected create PR and not create draft, not sure anyone would be confused and merge this as is in though so :) |
Pin/Delete/Export/Create Rules all work as expected
007b37f
to
bfb76a3
Compare
Nice stuff @mitchcapper! This is definitely a change I'm interested in and there's been lots of demand for this for a while, thanks for looking into it. I think there's a few things here that will need work to make this mergable though:
Does that all make sense? |
Agree with your points. Feedback on a few things:
|
Nope - if we do this right then I don't think any choice is required (if you don't want it, don't just select multiple requests and nothing will change) and I'm generally keen to avoid too much configuration possibilities, to keep both UX and the code simpler. Any users who really want to do funky things have full freedom to mess with the code themselves.
I think we're using slightly different definitions. The focus state I'm talking about is purely the raw web focus state, i.e. the focus that changes if you press tab, so that either a row is focused, or one of the buttons elsewhere on the page, or an input field, etc. It's totally independent of selection, and doesn't directly control the right pane. For rows, focus is currently shown as little red dotted outline. That moves if you click elsewhere (e.g. expand/collapse a section on the right) and moves with tab/shift-tab. When you click to multi-select, the last row clicked will end up focused, in that sense, but I wasn't suggesting that it would be shown on the right at all.
Given the above, this isn't quite right. I'm not sure what 'event data' is, but I think the only state we need is:
Regardless of the 'focus' terminology, this is a good suggestion (ditto for the equivalent deletion point). I think we can handle this within a multi-select right pane view though: for example, we could show "5 events selected" and then quick summaries of 5 rows there, which you could expand on mouseover with a little more detail. There are interesting UX options here we can certainly play with. So yes, that's a feature that totally makes sense to me, but I don't think it's strictly required for a usable first version, and most other software I've used that allows multi-selection doesn't have a solution to this. I think we should plan to improve later and keep this first iteration simple: if you have more than one item selected, the right pane always shows "N items selected" and buttons for bulk actions, nothing else. We can explore expansions and more complex behaviour in future, once we see get something working, see how it feels in practice, and get real feedback. |
|
e0e350c
to
1d713c8
Compare
14f95d3
to
94d8c06
Compare
Pin/Delete/Export/Create Rules all work as expected
Closes httptoolkit/httptoolkit#76
This adds a draft/sample for multi-select for rows, not sure if this is the direction one wants to go so certainly is not polished.
It implements:
Things that should be done:
Keep track of what rows are selected, this would prevent some of the constant enumerating, and more importantly give a count number of how many are selected that could be used other places (context menus, etc rather than saying 'Delete This Rule, Delete 5 rules', show on the right view pane also a count of how many are selected when > 1
Updating titles/context menu text to reflect multiple events selected
There is an actual checkbox mode can be enabled with setting USE_MULTI_SELECT_CHECKBOXES=true in view-event-list.ts. This enables standard checkboxes next to each row. This might be useful for people in the same way file explorer can have checkboxes enabled, also for mobile (is there mobile ?)
The multiSelected rows that are not the actual selection have the background/foreground set to the same as the selected row just without the bold. By default though this difference is hard to notice unless on high contrast, something should be changed for the CSS for this.
OnDelete might be slightly hokey given it runs all the delete logic for selecting the next event after deleting each event but seems to work (mass scale though might hit perf issues).
The export to HAR is hokey, by default the buttons for actions are the right side buttons, but there is no singular export to har button so I used the main button rather than add a new one. Downside is if you enable multi-select (check the box in the column header) then click on just one random event and go to export it exports one not all. Likely a har button added to the right side for when mutli-events are selected would be good.
It uses the filtered events for nearly all queries/actions. This may not make sense, especially where things that are filtered, then multiSelected, then not shown in the current filter stay selected. They should likely have the selection cleared. For what events most actions effect GetMultiselectSelectedBulkEvents handles the initial filtering to make it easier to adjust that pattern. view-event-list-buttons should use GetMultiselectSelectedBulkEvents but doesn't as I wasn't sure the best way to surface it other than passing the function all the way down the calls.
Right clicking to use a context menu when multi-select is enabled on a non multi-selected row can produce interesting results as it generally effects all selected items + row you are calling the menu on.
Technically you can multi-select some rows, control click a row then control + click it again to deselect it and the right pane goes away so you can't do most actions through that, even though there are still rows selected.
The checkbox alignment is the header uses some negative margin alignment. This was mainly done for when USE_MULTI_SELECT_CHECKBOXES is enabled it only takes up more space on each row when the checkbox in the header is clicked.
Keyboard multi-select was not implemented but is fairly straightforward.
Standard I don't know react so things certainly may not be done optimally. My few methods with actual in the name are poor form, but allows for minimal changes so should be refactored. The RowCheckbox in view-event-list in particular is a bit odd: first to be able to use a more contextually correct name of onChecked than onChanged for the event, I had to name it whenChanged otherwise react whines. Secondly I had to double specify the type for the props which I don't think I should have to do but couldn't figure out another way without creating the interface and using it.