CodeQL model editor: support saving single/selected models #3156
Conversation
There was a problem hiding this comment.
The code changes all look fine to me. When trying it out I did find it a little confusing to use, so I wonder if we should spend more time thinking about UX and get input from Asier before merging 🤔. I'm definitely feeling underqualified to say yes this is good to ship.
I wonder if changing the background colour is obvious enough as a way to select a row. Perhaps an icon or text is more intuitive? 🤔
I agree I'm not sure if it's obvious enough right now. But maybe it'll become more obvious if other bits of the UI mentioning selections are changing when you click on rows.
When you click anywhere in the method row (including on "View" or any of the dropdowns), it also selects/deselects the method row. Perhaps we can make just the method name clickable? 🤔 (How would that work?)
I also found this annoying. Particularly when using the dropdowns to edit a modeling it counts as "clicking on the row" and makes it selected or unselected. We can fix this particular behaviour with the dropdowns by calling event.preventDefault() in the on-click handlers of the dropdowns.
The "focused" property (from clicking "Review in editor") also temporarily highlights a row (until you click away). I've changed the "focused" background colour to an orange-y thing, but it might still be confusing:
Another data point is, because both "focussed" and "selected" work by changing the background colour, you can't tell if both are active. So after focussing a row it's unclear what would be saved at that moment.
Another thought I had was when using I wanted a way to "deselect all". Would it be helpful to add a button for this, maybe in the top-bar or in each section?
Also what do you think about deselecting everything after saving?
Thanks for the review! And yes, don't worry, I don't intend to merge this until the UI is sorted out 😅 I've asked internally for design improvements 🎨
I haven't been able to do this yet 🤔 I'll poke around a bit, but might need some more help 🙇🏽♀️
Both seem reasonable! I'll have a go at this too 🙌🏽 |
shati-patel
force-pushed
the
shati-patel/select-methods
branch
from
23ff34f
to
440e158
Compare
|
Co-authored-by: Robert <[email protected]>
shati-patel
force-pushed
the
shati-patel/select-methods
branch
from
a9dbb39
to
3f06c66
Compare
|
Having discussed with Could I get a final review? 🙏🏽 |
| inProgressMethods={inProgressMethods} | ||
| viewState={viewState} | ||
| hideModeledMethods={hideModeledMethods} | ||
| revealedMethodSignature={revealedMethodSignature} | ||
| onChange={onChange} | ||
| onMethodClick={onMethodClick} | ||
| /> | ||
| <SectionDivider /> | ||
| <ButtonsContainer> | ||
| <VSCodeButton onClick={handleSave} disabled={!hasUnsavedChanges}> |
There was a problem hiding this comment.
I spotted one odd piece of UI behaviour. If you have unsaved changes then the button will be enabled, but it could be that you haven't selected methods with unsaved changes, so pressing the button does nothing. It could be better if the button only became enabled when there were unsaved changes in the selected methods.
However I suggest we can leave this as a followup, since I believe we're expecting to do a design pass on this at some point anyway.
There was a problem hiding this comment.
Thanks for the review! That's a good point 🦅 👀
But I also agree that it can be a follow-up change instead, along with any other design/user feedback!
Adds the ability to click specific rows in the model editor, and just save those instead of the entire library/model. See internal linked issue for more details. (Paired with @robertbrignull 🍐)
Probably easiest to review commit-by-commit, since there's quite a bit of boilerplate.
📹 Demo
save-selected-methods.mp4
📝 Notes
I'm opening the PR now to get some general feedback on the approach, since we can change the styling more easily. I already have a few concerns/questions:
The "focused" property (from clicking "Review in editor") also temporarily highlights a row (until you click away). I've changed the "focused" background colour to an orange-y thing, but it might still be confusing:
focus-on-method-row.mp4
I wonder if changing the background colour is obvious enough as a way to select a row. Perhaps an icon or text is more intuitive? 🤔
When you click anywhere in the method row (including on "View" or any of the dropdowns), it also selects/deselects the method row. Perhaps we can make just the method name clickable? 🤔 (How would that work?)
Checklist
ready-for-doc-reviewlabel there.A docs update will probably be helpful. I'll hold off until we confirm the exact behaviour!Not needed for now