feat(query-bar): add resize handle and update execution key binding#7787
feat(query-bar): add resize handle and update execution key binding#7787bsradcliffe wants to merge 4 commits intomainfrom
Conversation
…keybinding Add a textarea-style resize grip to the filter field so users can manually expand the editor beyond the 10-line auto-grow limit. The grip uses role="separator" with ARIA value attributes for accessibility and supports keyboard resizing (Arrow keys, Home/End). Change the execute keybinding from Enter to Mod-Enter (Cmd+Enter on macOS, Ctrl+Enter on Windows/Linux) so bare Enter inserts a newline, aligning with the multiline editor model and the convention used by other database query tools. Co-authored-by: Cursor <cursoragent@cursor.com>
…y resized Remove the maxLines cap that was incorrectly added to the editor when the user has not resized. The original InlineEditor had no maxLines prop and grew with content indefinitely — passing maxLines=10 broke this by capping the editor at 10 lines with a scrollbar. Co-authored-by: Cursor <cursoragent@cursor.com>
bsradcliffe
changed the title
bsradcliffe
added
no-title-validation
The separator role is treated as non-interactive by eslint-plugin-jsx-a11y, causing no-noninteractive-element-interactions and no-noninteractive-tabindex errors. Switch to slider role which is interactive and semantically fits the resize handle with aria-valuenow/min/max. Also corrects orientation to vertical to match the up/down resize behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Me approving my own design review: |
| return [ | ||
| { | ||
| key: 'Enter', | ||
| key: 'Mod-Enter', |
There was a problem hiding this comment.
Do we have feedback from the Compass users that they are not happy with the current hotkey? Enter is a hotkey in compass for a long time (also why Shift+Enter is a new line and not just Enter), changing it as a drive-by if we don't have indicators that people are asking for this should probably be avoided
There was a problem hiding this comment.
It runs counter to what other tools do; Jakob's law. If users are used to Mod-Enter being the norm in their other tools, it's weird that we fight that. DataGrip, Databricks, Snowflake, Studio 3T, etc. eschew toward Mod-Enter. In any multi-line text area, I'd say most users expect Enter to create a newline. If Enter suddenly “runs” something, it’s easy to fire half-written queries by accident. Mod+Enter preserves the mental model: Enter edits, Mod+Enter executes.
This just maps to the user immediately saying "hey, I expect this input to act more like a multi-line input".
I understand this is changing something that has been in production for years, but I'd argue this is more a correcting alignment than a regression.
There was a problem hiding this comment.
I'm sure I'm not the only person that doesn't like when apps change established patterns as a minor, drive-by, change for no apparent reason. It's ultimately a product / design decision, but "someone else dose it that way" doesn't sound like a solid decision making strategy if no-one asked us to change that or complained abut Enter not being an expected combination
| const handleGripMouseDown = useCallback(() => { | ||
| const handleMouseMove = (event: MouseEvent) => { | ||
| setUserHeight((prev) => | ||
| clampHeight((prev ?? MIN_EDITOR_HEIGHT) + event.movementY) |
There was a problem hiding this comment.
Style updates like that shouldn't go through React state (there are cases where it might be needed, but that's not one of them): this is firing too often and causing the whole React rendering cycle to kick off every time. Instead you should update the container element styles directly
There was a problem hiding this comment.
Understood, so just targeting inline styles? I definitely don't want to trigger re-renders because I've notice how laggy/degraded behavior becomes when that happens.
| }); | ||
|
|
||
| // Matches BaseEditor's default lineHeight prop value. | ||
| const EDITOR_LINE_HEIGHT = 16; |
There was a problem hiding this comment.
Let's avoid random magic numbers. You can define these in compass-edtor and re-export them from the package. That way the values are located in their logial place and will stay up to date if changed
There was a problem hiding this comment.
I'll take a look at that package and see the convention we currently follow. Thanks!
|
|
||
| // Matches BaseEditor's default lineHeight prop value. | ||
| const EDITOR_LINE_HEIGHT = 16; | ||
| const MAX_EDITOR_LINES = 50; |
There was a problem hiding this comment.
Don't mind this being bigger than it is right now, but the current value is 10 and it was chosen as a good compromise between allowing more lines to show vs taking too much space on the screen. 50 allows you to do silly stuff like this (this is just one field):
... and scrolling doesn't work when this happens
There was a problem hiding this comment.
I think 20 is as high as I'd want to go. This is a good edge case to consider, and I definitely don't want the input's growth making scrolling impossible to render.
| ref={editorContainerRef} | ||
| style={ | ||
| userHeight !== null | ||
| ? { height: userHeight, minHeight: MIN_EDITOR_HEIGHT } |
There was a problem hiding this comment.
The way you implemented the resizing, it completely breaks the existing editor behavor that auto expands the editor when new lines are added
There was a problem hiding this comment.
Eh? Let me look again. I had noticed this myself but I was pretty sure I went back and fixed it. Checking it out again.
| /> | ||
| {!disabled && optionName === 'filter' && ( | ||
| <div | ||
| role="slider" |
There was a problem hiding this comment.
Slider usually support left and right in addition to up and down as a way to increase / decrease the value (it doesn't depend oon the orientation attr). I recommend reading through the documentation for the role where it's called out
| onBlur={onBlur} | ||
| maxLines={maxLines} | ||
| /> | ||
| {!disabled && optionName === 'filter' && ( |
There was a problem hiding this comment.
Why wouldn't you want to change the size if the editor is disabled for some reason?
There was a problem hiding this comment.
You mean if the primary filter input is disabled, why wouldn't you want to resize it? We typically disable all interactions with a field if that's the case.
| // Matches the LeafyGreen "Resize" glyph — two diagonal lines in the corner. | ||
| const resizeGripSvg = encodeURIComponent( |
There was a problem hiding this comment.
If this is for a real element we are rendering on the screen why are we inline hardcoding the icon instead of using leafygreen icon component? There doesn't seem to be a reason to set it as a background image compared to just using Icon component inside your slider container
There was a problem hiding this comment.
Wasn't sure what the convention was for slotting things within. Can definitely refactor to import the Resize icon here.
|
@gribnoysup will follow up with you here, but thanks for the guidance! This is exactly the type of feedback I'm looking for. I think design is going to be looking for more established touchpoints with engineering for stuff like this. |
gribnoysup
added
release notes
and removed
no release notes
gribnoysup
changed the title
2 participants
Description
I inlined a resize handler to the query bar that CodeMirror does not natively support. It can be made DRY in the future if we need to extend resizing to other fields, but this is the only surface that needs to ingest it right now, so inlining made sense.
Additionally, we did not follow the standard key combination for executing a query. Most IDEs/editors use "Ctrl/Cmd+Enter" to fire a query. Instead, we used "Enter", which is typically reserved for creating new lines and is likely to help discoverability of the auto-growing "Find" input. This changes CodeMirror on the documents tab to match this industry paradigm.
Checklist
Motivation and Context
https://mongodb.slack.com/archives/GDTJXPHD0/p1770825572519319
Open Questions
Dependents
Types of changes