Open mitchcapper opened 10 months ago
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 :)
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:
OnDelete
here for example uses either an event
argument or a GetMultiselectSelectedBulkEvents
list of events when multi-select is active. Instead it should just always take a list of events and delete them.selectedEvent
should be selectedEvents
, and should always return a list of 0+ events, using some state stored in that component./view/multi
) for example.Does that all make sense?
Agree with your points. Feedback on a few things:
the global MULTI_SELECT bool was more showing a way the entire feature could be turned on and off but similar can just be done in the select handler if you decide to give users a choice.
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 selected and focused should be distinct as you mention.
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.
if you do go the route of focus / select being distinct I assume we modify event data to transmit both rather than just saying the last item on the list (or what have you) is the focused item. this also solved the url question leaving it to point to the focused item
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:
/view/$X
) that's either blank (nothing selected), a single event ID, or multi
.
/view/<breakpointed event id>
)If I am trying to export several queries I may need to see a request to know if I want to add it.
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.
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.
Pin/Delete/Export/Create Rules all work as expected Closes https://github.com/httptoolkit/httptoolkit/issues/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.