fix: handle horizontal scroll in TextField when text is long#3927
Merged
Conversation
Signed-off-by: Adriano Lazzaretti <lazzaretti136@gmail.com>
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- For the multiline
InlineTextField,Entercurrently triggerssave()and prevents default, which makes it impossible to insert line breaks; if the intent is true multiline editing, consider supportingShift+Enter(or similar) to insert newlines while keepingEnteras save. - In the multiline path you both set
autoFocuson the<textarea>and imperatively callfocus()in auseEffect; you can likely drop one of these to avoid redundant focus handling and potential focus flicker.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the multiline `InlineTextField`, `Enter` currently triggers `save()` and prevents default, which makes it impossible to insert line breaks; if the intent is true multiline editing, consider supporting `Shift+Enter` (or similar) to insert newlines while keeping `Enter` as save.
- In the multiline path you both set `autoFocus` on the `<textarea>` and imperatively call `focus()` in a `useEffect`; you can likely drop one of these to avoid redundant focus handling and potential focus flicker.
## Individual Comments
### Comment 1
<location> `turboui/src/TextField/index.tsx:294-297` </location>
<code_context>
+ onChange={(e) => state.setCurrentText(e.target.value)}
+ onBlur={state.save}
+ onKeyDown={handleKeyDown}
+ autoFocus
+ rows={1}
+ wrap="soft"
</code_context>
<issue_to_address>
**suggestion:** Redundant focus handling for multiline (autoFocus + effect) could be simplified and made consistent.
In the textarea case you use both `autoFocus` and an effect that calls `textareaRef.current.focus()` when `state.isEditing` is true, while the input relies only on the effect. This duplication can make focus behavior harder to reason about as `isEditing` changes. Consider using just the effect for both branches for consistent, single-source focus control.
```suggestion
onKeyDown={handleKeyDown}
rows={1}
wrap="soft"
```
</issue_to_address>
### Comment 2
<location> `turboui/src/TextField/index.tsx:267` </location>
<code_context>
}
};
+ const wrapperClass = state.multiline ? "min-w-0" : state.className;
+
return (
</code_context>
<issue_to_address>
**suggestion (bug_risk):** In multiline mode, the outer wrapper no longer receives `state.className`, which may change how callers can style the component.
In the previous behavior, `state.className` was applied to the outer `<div>`, letting callers control layout of the whole field (including label and click target). With `wrapperClass = state.multiline ? "min-w-0" : state.className`, multiline mode now only applies `state.className` to the input/textarea. If this change isn’t intentional, you could preserve the old styling surface with something like:
```ts
const wrapperClass = state.multiline
? classNames("min-w-0", state.className)
: state.className;
```
```suggestion
const wrapperClass = state.multiline
? classNames("min-w-0", state.className)
: state.className;
```
</issue_to_address>
### Comment 3
<location> `turboui/src/TextField/index.tsx:173` </location>
<code_context>
}
-function InlineTextField(state: TextField.State & { inputRef?: React.Ref<HTMLInputElement> }) {
+function InlineTextField(
+ state: TextField.State & { inputRef?: React.Ref<HTMLInputElement | HTMLTextAreaElement>; multiline: boolean },
+) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting InlineTextField into separate single-line and multiline subcomponents so each mode manages its own refs, effects, and JSX without multiline conditionals scattered throughout.
You can keep the new multiline functionality but reduce complexity by splitting the “mode-specific” logic out of `InlineTextField` and eliminating most `multiline` conditionals from that component.
### 1. Split into mode-specific subcomponents
Instead of a single `InlineTextField` that branches on `state.multiline` in effects, JSX, classNames, and refs, delegate to two internal components. This keeps the public API the same but moves complexity out of the shared path:
```tsx
function InlineTextField(
state: TextField.State & {
inputRef?: React.Ref<HTMLInputElement | HTMLTextAreaElement>;
multiline: boolean;
},
) {
if (state.multiline) {
return <InlineTextFieldMultiline {...state} />;
}
return <InlineTextFieldSingleLine {...state} />;
}
```
Now each mode-specific component can own its own refs, effects, and layout without `state.multiline` checks everywhere.
### 2. Single-line: keep existing behavior, simplify effects
Move the single-line logic into its own component; notice that we no longer need `multiline` checks or branching in effects:
```tsx
function InlineTextFieldSingleLine(
state: TextField.State & {
inputRef?: React.Ref<HTMLInputElement | HTMLTextAreaElement>;
},
) {
const inputRef = useRef<HTMLInputElement | null>(null);
const hiddenSpanRef = useRef<HTMLSpanElement | null>(null);
useEffect(() => {
assignRef(state.inputRef, inputRef.current);
}, [state.inputRef]);
const adjustInputWidth = () => {
if (!hiddenSpanRef.current || !inputRef.current) return;
const textToMeasure = state.currentText || state.placeholder || " ";
hiddenSpanRef.current.textContent = textToMeasure;
const textWidth = hiddenSpanRef.current.offsetWidth;
const desiredWidth = Math.max(textWidth + 10, 50);
inputRef.current.style.width = `${desiredWidth}px`;
inputRef.current.style.maxWidth = "100%";
};
useEffect(() => {
if (!state.isEditing || !inputRef.current) return;
inputRef.current.focus();
adjustInputWidth();
}, [state.isEditing]);
useEffect(() => {
if (!state.isEditing) return;
adjustInputWidth();
}, [state.currentText, state.isEditing]);
// ... keep existing single-line JSX as-is (no multiline ternaries) ...
}
```
This removes:
- `state.multiline` from effects
- the inline `state.multiline ? <textarea> : <input>` ternary for this mode
- multiline-related className conditions
### 3. Multiline: isolate textarea-specific behavior
Likewise, move the multiline logic into its own component with only textarea-specific effects and layout:
```tsx
function InlineTextFieldMultiline(
state: TextField.State & {
inputRef?: React.Ref<HTMLInputElement | HTMLTextAreaElement>;
},
) {
const textareaRef = useRef<HTMLTextAreaElement | null>(null);
useEffect(() => {
assignRef(state.inputRef, textareaRef.current);
}, [state.inputRef]);
const autoResizeTextarea = () => {
const el = textareaRef.current;
if (!el) return;
el.style.height = "auto";
el.style.height = `${el.scrollHeight}px`;
};
useEffect(() => {
if (!state.isEditing || !textareaRef.current) return;
textareaRef.current.focus();
autoResizeTextarea();
}, [state.isEditing]);
useEffect(() => {
if (!state.isEditing) return;
autoResizeTextarea();
}, [state.currentText, state.isEditing]);
useEffect(() => {
if (!state.isEditing) return;
const el = textareaRef.current;
if (!el || document.activeElement !== el) return;
const length = el.value.length;
try {
el.setSelectionRange(length, length);
} catch {
// ignore
}
}, [state.isEditing]);
const handleKeyDown = (e: React.KeyboardEvent) => {
if (e.key === "Escape") {
state.cancel();
} else if (e.key === "Enter") {
e.preventDefault();
state.save();
}
};
// ... multiline-only JSX (no ternary between textarea/input) ...
}
```
This removes:
- `state.multiline` guards from three `useEffect` hooks
- `state.multiline` branches in `handleKeyDown`
- `state.multiline`-dependent outer / wrapper class logic inside this component (just define the multiline layout directly)
### 4. Simplify JSX and className logic
After splitting, each component can have linear, mode-specific render logic:
```tsx
// Single-line
const outerClass = classNames(
"cursor-text relative inline-block",
{
"hover:bg-surface-dimmed": !state.isEditing && !state.readonly,
"bg-surface-dimmed": state.isEditing,
"rounded px-1 py-0.5 -mx-1 -my-0.5": true,
},
);
// Multiline
const outerClass = classNames(
"cursor-text relative block w-full",
// no hover/bg toggling if that’s not desired for multiline
);
```
```tsx
// Single-line editing view
return state.isEditing ? (
<input /* ... */ />
) : (
<span /* ... */ />
);
// Multiline editing view
return state.isEditing ? (
<textarea /* ... */ />
) : (
<span
className={state.className}
style={{
color: !state.currentText && state.placeholder ? "var(--color-content-subtle)" : undefined,
whiteSpace: "pre-wrap",
overflowWrap: "anywhere",
}}
>
{state.currentText || state.placeholder || " "}
</span>
);
```
This eliminates the nested ternary:
```tsx
state.isEditing
? (state.multiline ? <textarea> : <input>)
: <span>
```
and keeps the read-only span styling tailored to each mode.
---
These changes keep the public API, current behavior (including caret behavior, auto-resize, and key handling), and new multiline support, but significantly reduce conditional complexity and make each code path easier to reason about and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.