-
Notifications
You must be signed in to change notification settings - Fork 258
feat(query-bar): add resize handle and update execution key binding #7787
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?
Changes from all commits
b1b00f2
310e87f
0639f4b
a3365dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, { useMemo, useRef } from 'react'; | ||
| import React, { useCallback, useMemo, useRef, useState } from 'react'; | ||
| import { | ||
| css, | ||
| cx, | ||
|
|
@@ -76,6 +76,59 @@ const editorWithErrorStyles = css({ | |
| }, | ||
| }); | ||
|
|
||
| // Matches BaseEditor's default lineHeight prop value. | ||
| const EDITOR_LINE_HEIGHT = 16; | ||
| const MAX_EDITOR_LINES = 50; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| // The initial rendered height of the editor container (single line of content | ||
| // + cm-content padding + container padding/border). Acts as the floor for | ||
| // the resize grip — the editor should never shrink below this. | ||
| const MIN_EDITOR_HEIGHT = 28; | ||
| const MAX_EDITOR_HEIGHT = MAX_EDITOR_LINES * EDITOR_LINE_HEIGHT; | ||
|
|
||
| function clampHeight(h: number) { | ||
| return Math.max(MIN_EDITOR_HEIGHT, Math.min(MAX_EDITOR_HEIGHT, h)); | ||
| } | ||
|
|
||
| // Matches the LeafyGreen "Resize" glyph — two diagonal lines in the corner. | ||
| const resizeGripSvg = encodeURIComponent( | ||
|
Comment on lines
+93
to
+94
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't sure what the convention was for slotting things within. Can definitely refactor to import the Resize icon here. |
||
| `<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">` + | ||
| `<path fill-rule="evenodd" clip-rule="evenodd" ` + | ||
| `d="M14.7706 5.71967C15.0631 6.01256 15.0631 6.48744 14.7706 6.78033` + | ||
| `L6.77898 14.7803C6.4864 15.0732 6.01202 15.0732 5.71944 14.7803` + | ||
| `C5.42685 14.4874 5.42685 14.0126 5.71944 13.7197L13.711 5.71967` + | ||
| `C14.0036 5.42678 14.478 5.42678 14.7706 5.71967Z" ` + | ||
| `fill="${palette.gray.base}"/>` + | ||
| `<path fill-rule="evenodd" clip-rule="evenodd" ` + | ||
| `d="M14.7806 10.2197C15.0731 10.5126 15.0731 10.9874 14.7806 11.2803` + | ||
| `L11.2842 14.7803C10.9917 15.0732 10.5173 15.0732 10.2247 14.7803` + | ||
| `C9.93212 14.4874 9.93212 14.0126 10.2247 13.7197L13.721 10.2197` + | ||
| `C14.0136 9.92678 14.488 9.92678 14.7806 10.2197Z" ` + | ||
| `fill="${palette.gray.base}"/>` + | ||
| `</svg>` | ||
| ); | ||
|
|
||
| const resizeGripStyles = css({ | ||
| position: 'absolute', | ||
| bottom: 0, | ||
| right: 0, | ||
| width: 16, | ||
| height: 16, | ||
| padding: 0, | ||
| border: 'none', | ||
| background: 'transparent', | ||
| backgroundImage: `url("data:image/svg+xml,${resizeGripSvg}")`, | ||
| backgroundSize: '100% 100%', | ||
| cursor: 'ns-resize', | ||
| opacity: 0.6, | ||
| zIndex: 100, | ||
| outline: 'none', | ||
| transition: 'opacity 150ms ease', | ||
| '&:hover, &:focus-visible': { | ||
| opacity: 1, | ||
| }, | ||
| }); | ||
|
|
||
| type OptionEditorProps = { | ||
| optionName: QueryOptionOfTypeDocument; | ||
| namespace: string; | ||
|
|
@@ -128,13 +181,69 @@ export const OptionEditor: React.FunctionComponent<OptionEditorProps> = ({ | |
|
|
||
| const darkMode = useDarkMode(); | ||
|
|
||
| // Tracks the user's manual resize height in pixels. When null, the editor | ||
| // auto-grows with content (no cap). Once the user drags the resize grip, | ||
| // this holds the pixel height for the container. | ||
| const [userHeight, setUserHeight] = useState<number | null>(null); | ||
|
|
||
| const handleGripMouseDown = useCallback(() => { | ||
| const handleMouseMove = (event: MouseEvent) => { | ||
| setUserHeight((prev) => | ||
| clampHeight((prev ?? MIN_EDITOR_HEIGHT) + event.movementY) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| ); | ||
| }; | ||
|
|
||
| const handleMouseUp = () => { | ||
| document.removeEventListener('mousemove', handleMouseMove); | ||
| document.removeEventListener('mouseup', handleMouseUp); | ||
| }; | ||
|
|
||
| document.addEventListener('mousemove', handleMouseMove); | ||
| document.addEventListener('mouseup', handleMouseUp); | ||
| }, []); | ||
|
|
||
| const handleGripKeyDown = useCallback((event: React.KeyboardEvent) => { | ||
| const step = EDITOR_LINE_HEIGHT; | ||
| let handled = true; | ||
|
|
||
| switch (event.key) { | ||
| case 'ArrowDown': | ||
| setUserHeight((prev) => | ||
| clampHeight((prev ?? MIN_EDITOR_HEIGHT) + step) | ||
| ); | ||
| break; | ||
| case 'ArrowUp': | ||
| setUserHeight((prev) => | ||
| clampHeight((prev ?? MIN_EDITOR_HEIGHT) - step) | ||
| ); | ||
| break; | ||
| case 'End': | ||
| setUserHeight(MAX_EDITOR_HEIGHT); | ||
| break; | ||
| case 'Home': | ||
| setUserHeight(MIN_EDITOR_HEIGHT); | ||
| break; | ||
| default: | ||
| handled = false; | ||
| } | ||
|
|
||
| if (handled) { | ||
| event.preventDefault(); | ||
| } | ||
| }, []); | ||
|
|
||
| // When the user has manually resized, cap maxLines high so the container's | ||
| // CSS height is the actual constraint. Otherwise omit it to preserve the | ||
| // original auto-grow behavior (editor grows with content, no cap). | ||
| const maxLines = userHeight !== null ? MAX_EDITOR_LINES : undefined; | ||
|
|
||
| const onApplyRef = useRef(onApply); | ||
| onApplyRef.current = onApply; | ||
|
|
||
| const commands = useMemo<Command[]>(() => { | ||
| return [ | ||
| { | ||
| key: 'Enter', | ||
| key: 'Mod-Enter', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| run() { | ||
| onApplyRef.current?.(); | ||
| return true; | ||
|
|
@@ -244,6 +353,11 @@ export const OptionEditor: React.FunctionComponent<OptionEditorProps> = ({ | |
| hasError && editorWithErrorStyles | ||
| )} | ||
| ref={editorContainerRef} | ||
| style={ | ||
| userHeight !== null | ||
| ? { height: userHeight, minHeight: MIN_EDITOR_HEIGHT } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way you implemented the resizing, it completely breaks the existing editor behavor that auto expands the editor when new lines are added
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| : undefined | ||
| } | ||
| > | ||
| <InlineEditor | ||
| ref={editorRef} | ||
|
|
@@ -258,7 +372,22 @@ export const OptionEditor: React.FunctionComponent<OptionEditorProps> = ({ | |
| onFocus={onFocus} | ||
| onPaste={onPaste} | ||
| onBlur={onBlur} | ||
| maxLines={maxLines} | ||
| /> | ||
| {!disabled && optionName === 'filter' && ( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why wouldn't you want to change the size if the editor is disabled for some reason?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| <div | ||
| role="slider" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| aria-orientation="vertical" | ||
| aria-valuenow={userHeight ?? MIN_EDITOR_HEIGHT} | ||
| aria-valuemin={MIN_EDITOR_HEIGHT} | ||
| aria-valuemax={MAX_EDITOR_HEIGHT} | ||
| aria-label="Resize query editor" | ||
| tabIndex={0} | ||
| className={resizeGripStyles} | ||
| onMouseDown={handleGripMouseDown} | ||
| onKeyDown={handleGripKeyDown} | ||
| /> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at that package and see the convention we currently follow. Thanks!