Skip to content
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

feat: support multiline for EditableText #2683

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
edb0a8f
add multiline and support having a textarea instead of input
gabriel-amram Dec 24, 2024
ba0691d
feat: allow multilines for editable text. No breaking changes to exis…
gabriel-amram Dec 24, 2024
3cf410b
feat: allow multilines for editable text. No breaking changes to exis…
gabriel-amram Dec 24, 2024
4aac9b0
linting
gabriel-amram Dec 24, 2024
884570f
l
gabriel-amram Dec 24, 2024
0a560dd
Merge branch 'gabrielam/add-multiline-to-editable-text' of https://gi…
gabriel-amram Dec 24, 2024
fb6e211
fix some styling and calculating height issues
gabriel-amram Dec 25, 2024
701c3a7
don't change the ref explicitly, but use the state
gabriel-amram Dec 26, 2024
9eb4a4e
shift-enter creates the new line, while simple enter defocuses
gabriel-amram Dec 26, 2024
cbb1812
added and fixed tests
gabriel-amram Dec 26, 2024
0440768
Merge branch 'master' into gabrielam/add-multiline-to-editable-text
gabriel-amram Dec 26, 2024
61a9790
CR comments
gabriel-amram Dec 29, 2024
5431d50
Merge branch 'gabrielam/add-multiline-to-editable-text' of https://gi…
gabriel-amram Dec 29, 2024
dd72841
reduce rendering by using inline
gabriel-amram Dec 30, 2024
2e29cde
reduce renders, cache the textarea attrs
gabriel-amram Dec 30, 2024
f7aace2
remove comments
gabriel-amram Dec 30, 2024
7be9a14
use pre-wrap for line breaks
gabriel-amram Dec 31, 2024
c2210dc
sync with origin
gabriel-amram Dec 31, 2024
3c6e3b9
Merge branch 'master' into gabrielam/add-multiline-to-editable-text
gabriel-amram Dec 31, 2024
a4c5cb6
minor cr notes
gabriel-amram Dec 31, 2024
f9269c4
Merge branch 'gabrielam/add-multiline-to-editable-text' of https://gi…
gabriel-amram Dec 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/core/src/components/EditableText/EditableText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,24 @@ export interface EditableTextProps extends VibeComponentProps, EditableTypograph
/** Sets the Text weight
*/
weight?: TextWeight;
/** Enables editing multiple lines of text
*/
multiline?: boolean;
}

const EditableText: VibeComponent<EditableTextProps, HTMLElement> & {
types?: typeof TextTypeEnum;
weights?: typeof TextWeightEnum;
} = forwardRef(
(
{ type = "text2", weight = "normal", "data-testid": dataTestId, id, ...editableTypographyProps }: EditableTextProps,
{
type = "text2",
weight = "normal",
"data-testid": dataTestId,
id,
multiline,
...editableTypographyProps
}: EditableTextProps,
ref
) => {
return (
Expand All @@ -38,6 +48,7 @@ const EditableText: VibeComponent<EditableTextProps, HTMLElement> & {
clearable
type={type}
weight={weight}
multiline={multiline}
{...editableTypographyProps}
/>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ Editable text can be used with any of the <StorybookLink page="Text/Text">Text</

<Canvas of={EditableTextStories.Types} />

### Multiline

Editable text can be used to allow multiline input.

<Canvas of={EditableTextStories.Multiline} />

## Related components

<RelatedComponents componentsNames={[TEXT, HEADING, EDITABLE_HEADING]} />
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,9 @@
}
}
}

.multilineContainer {
gabriel-amram marked this conversation as resolved.
Show resolved Hide resolved
.editableText {
width: 400px;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,24 @@ export const Types = {
}
}
};

export const Multiline = {
render: () => (
<div className={styles.multilineContainer}>
<EditableText
type={EditableText.types.TEXT1}
weight={EditableText.weights.NORMAL}
multiline
value={"This is a multiline\nhere's the second line"}
className={styles.editableText}
talkor marked this conversation as resolved.
Show resolved Hide resolved
/>
</div>
),
parameters: {
docs: {
liveEdit: {
scope: { styles }
}
}
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ describe("EditableText", () => {
expect(input).toBeInTheDocument();
});

it("should render a textarea in edit mode with multiline", () => {
render(<EditableText multiline value="Editable text" />);

const component = screen.getByRole("button");
fireEvent.click(component);

const input = screen.queryByRole("textbox");
expect(input).toBeInTheDocument();
});

it("should not render an input when 'readOnly' is false when clicked", () => {
render(<EditableText value="Editable test" readOnly />);

Expand Down Expand Up @@ -98,6 +108,29 @@ describe("EditableText", () => {
expect(onChange).toHaveBeenCalledWith(newValue);
});

it("should call onChange with new value when changed in a multiline editable component", async () => {
const value = "Editable test";
const newValue = "New Editable test";
render(<EditableText value={value} onChange={onChange} multiline />);

const component = screen.getByRole("button");
fireEvent.click(component);

const input = screen.getByRole("textbox");
fireEvent.change(input, {
target: { value: newValue }
});

expect(input).toHaveValue(newValue);

await waitFor(() => {
fireEvent.keyDown(input, { key: "Enter" });
});
expect(within(await screen.findByRole("button")).getByText(newValue)).toBeInTheDocument();
expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith(newValue);
});

it("should not call onChange when value isn't changed in an editable component", async () => {
const value = "Editable test";
render(<EditableText value={value} onChange={onChange} />);
Expand All @@ -118,6 +151,24 @@ describe("EditableText", () => {
expect(onChange).not.toBeCalled();
});

it("should not call onChange when value changed but Shift+Enter was clicked for multiline in an editable component", async () => {
const value = "Editable test";
render(<EditableText value={value} onChange={onChange} multiline />);

const component = screen.getByRole("button");
fireEvent.click(component);

const input = screen.getByRole("textbox");

expect(input).toHaveValue(value);

await waitFor(() => {
fireEvent.keyDown(input, { key: "Enter", shiftKey: true });
});

expect(onChange).not.toBeCalled();
});

it("should not call onChange when value of an editable component is cleared", async () => {
const value = "Editable test";
render(<EditableText value={value} onChange={onChange} />);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
overflow: hidden;
position: relative;

.input {
.input,.textarea {
display: inline-block;
max-width: 100%;
min-width: 64px;
Expand All @@ -26,6 +26,10 @@
}
}

.textarea {
resize: none;
}

.typography {
border: 1px solid transparent;
padding: 0 var(--spacing-xs);
Expand All @@ -46,5 +50,9 @@
&.placeholder {
color: var(--secondary-text-color);
}

&.multiline {
white-space: pre-wrap;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ export interface EditableTypographyProps extends VibeComponentProps, EditableTyp
type?: TextType | HeadingType;
/** Sets the Text/Heading weight */
weight?: TextWeight | HeadingWeight;
/** Controls whether a textarea or a simple input would be rendered, allowing multi-lines */
multiline?: boolean;
talkor marked this conversation as resolved.
Show resolved Hide resolved
}

const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> = forwardRef(
Expand All @@ -67,7 +69,8 @@ const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> =
onEditModeChange,
tooltipProps,
type,
weight
weight,
multiline = false
}: EditableTypographyProps,
ref
) => {
Expand All @@ -77,6 +80,9 @@ const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> =
const [isEditing, setIsEditing] = useState(isEditMode || false);
const [inputValue, setInputValue] = useState(value);
const [inputWidth, setInputWidth] = useState(0);
const [inputHeight, setInputHeight] = useState<number | string>(0);
const textareaBorderBoxSizing = useRef(0);
const textareaLineHeight = useRef(0);

const prevValue = usePrevious(value);

Expand Down Expand Up @@ -131,8 +137,12 @@ const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> =
handleInputValueChange();
}

function handleKeyDown(event: React.KeyboardEvent<HTMLInputElement>) {
function handleKeyDown(event: React.KeyboardEvent<HTMLInputElement | HTMLTextAreaElement>) {
if (event.key === keyCodes.ENTER) {
if (multiline && event.shiftKey) {
return;
}

handleInputValueChange();
}
if (event.key === keyCodes.ESCAPE) {
Expand All @@ -141,14 +151,46 @@ const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> =
}
}

function handleChange(event: React.ChangeEvent<HTMLInputElement>) {
function handleChange(event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>) {
setInputValue(event.target.value);

if (multiline) {
resizeTextarea();
}
}

const toggleKeyboardEditMode = useKeyboardButtonPressedFunc(toggleEditMode);

function focus() {
inputRef.current?.focus?.();

if (multiline) {
calculateTextareaHeightAttrs();
}
}

/* Dynamically resizes the textarea to fit its content */
function resizeTextarea() {
gabriel-amram marked this conversation as resolved.
Show resolved Hide resolved
if (inputRef.current) {
// Temporarily set the height to "auto" to accurately measure the scroll height of the content inside the textarea.
setInputHeight("auto");
gabriel-amram marked this conversation as resolved.
Show resolved Hide resolved

requestAnimationFrame(() => {
gabriel-amram marked this conversation as resolved.
Show resolved Hide resolved
const textarea = inputRef.current as HTMLTextAreaElement;

if (!textarea) {
return;
}

// Ensure we at least have 1 line
setInputHeight(
Math.max(
textarea.scrollHeight + textareaBorderBoxSizing.current,
textareaLineHeight.current + textareaBorderBoxSizing.current
gabriel-amram marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +192 to +195
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thought, looks like the only difference in the dimensions is 2px, and there's no difference in paddings/line heights etc between the different variants, so looks like doing just:
setInputHeight(textarea.scrollHeight + 2); works exactly the same and it can save the getComputeStyle and the other calculations in calculateTextareaHeightAttrs (which already have some default static numbers). Giving it auto in the beginning will already make it take the the currect box size. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the question gets back to you :)
technically its only 2px because the border-top/bottom-width is 1px (or 2px overall)
border-box should have included the border as well as the padding, but it doesn't

do we want to just rely on always having this border/padding setting and that the user cannot add anything on their own using overwriting css?
@talkor

Copy link
Member

@talkor talkor Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it is border-box, it's just that scrollHeight is based purely on the content and padding of the element's scrollable content, without the borders.
We don't want users to override CSS as it can make design inconsistent so I wouldn't take it into consideration, generally speaking we support the variants we officially provide so it's ok :)
@gabriel-amram

)
);
});
}
}

function selectAllInputText() {
Expand All @@ -167,10 +209,38 @@ const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> =
if (!typographyRef.current) {
return;
}

const { width } = typographyRef.current.getBoundingClientRect();
setInputWidth(width);
}, [inputValue, isEditing]);

/* Calculate the minimual textarea height, taking its applied styles (padding, border width) into consideration
This is done only on focus, so that we don't need to get the computed style every time.
*/
function calculateTextareaHeightAttrs() {
if (multiline && inputRef.current) {
const textarea = inputRef.current as HTMLTextAreaElement;

if (!textarea) {
return;
}

const computedStyle = window.getComputedStyle(textarea);

// Calculate the appropriate height by taking into account the scrollable content inside the textarea,
// as well as the styles applied to it, such as padding and border widths.
const lineHeight = parseFloat(computedStyle.lineHeight) || 16;
const paddingTop = parseFloat(computedStyle.paddingTop) || 0;
const paddingBottom = parseFloat(computedStyle.paddingBottom) || 0;
const borderTopWidth = parseFloat(computedStyle.borderTopWidth) || 0;
const borderBottomWidth = parseFloat(computedStyle.borderBottomWidth) || 0;

textareaLineHeight.current = lineHeight;
textareaBorderBoxSizing.current = paddingTop + paddingBottom + borderTopWidth + borderBottomWidth;
resizeTextarea();
}
}

return (
<div
talkor marked this conversation as resolved.
Show resolved Hide resolved
ref={mergedRef}
Expand All @@ -182,32 +252,49 @@ const EditableTypography: VibeComponent<EditableTypographyProps, HTMLElement> =
onClick={onTypographyClick}
onKeyDown={toggleKeyboardEditMode}
>
{isEditing && (
<input
ref={inputRef}
className={cx(styles.input, typographyClassName)}
value={inputValue}
onChange={handleChange}
onKeyDown={handleKeyDown}
onBlur={handleBlur}
aria-label={ariaLabel}
placeholder={placeholder}
style={{ width: inputWidth }}
role="input"
/>
)}
{isEditing &&
talkor marked this conversation as resolved.
Show resolved Hide resolved
(multiline ? (
<textarea
ref={inputRef}
className={cx(styles.textarea, typographyClassName)}
value={inputValue}
onChange={handleChange}
onKeyDown={handleKeyDown}
onBlur={handleBlur}
aria-label={ariaLabel}
placeholder={placeholder}
style={{ width: inputWidth, height: inputHeight }}
role="textbox"
rows={1}
/>
) : (
<input
ref={inputRef}
className={cx(styles.input, typographyClassName)}
value={inputValue}
onChange={handleChange}
onKeyDown={handleKeyDown}
onBlur={handleBlur}
aria-label={ariaLabel}
placeholder={placeholder}
style={{ width: inputWidth }}
role="input"
/>
))}
<TypographyComponent
ref={typographyRef}
aria-hidden={isEditing}
className={cx(styles.typography, typographyClassName, {
[styles.hidden]: isEditing,
[styles.disabled]: readOnly,
[styles.placeholder]: !inputValue && placeholder
[styles.placeholder]: !inputValue && placeholder,
[styles.multiline]: !isEditing && multiline,
})}
tabIndex={0}
tooltipProps={tooltipProps}
weight={weight}
type={type}
ellipsis={!multiline}
>
{inputValue || placeholder}
</TypographyComponent>
Expand Down
Loading