Skip to content

[WIP] [EuiInlineEdit] Allow EuiInlineEdit to be used as a controlled component #7117

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.

Sign up for GitHub

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

Closed
breehall wants to merge 3 commits into from
Closed

[WIP] [EuiInlineEdit] Allow EuiInlineEdit to be used as a controlled component #7117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d71c36
[EuiInlineEdit] Create two new props to allow inline edit to be used …
breehall Aug 23, 2023
1dab8e3
[EuiInlineEdit] Controlled component:
breehall Aug 23, 2023
51ad584
[DEMO ONLY] Create a demo of controlled component usage inside of Eui…
breehall Aug 23, 2023
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
17 changes: 17 additions & 0 deletions src-docs/src/views/inline_edit/inline_edit_title.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ export default () => {
setToggleTitleButtonSize(optionId);
};

const [inlineEditValue, setInlineEditValue] = useState('hello world');

return (
<>
<EuiButtonGroup
Expand All @@ -59,6 +61,21 @@ export default () => {
inputAriaLabel="Edit title inline"
defaultValue="Hello World (but as a title)!"
/>

<EuiSpacer />

<EuiInlineEditTitle
heading="h3"
size={toggleTitleButtonSize}
inputAriaLabel="Edit title inline"
value={inlineEditValue}
onChange={(e) => {
setInlineEditValue(e.target.value);
}}
onCancel={(previousValue) => {
setInlineEditValue(previousValue);
}}
Comment on lines +71 to +77

@cee-chen cee-chen Aug 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👌💋 B-E-A-utiful developer experience!

/>
</>
);
};
49 changes: 39 additions & 10 deletions src/components/inline_edit/inline_edit_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ import React, {
useState,
useRef,
useEffect,
useMemo,
HTMLAttributes,
MouseEvent,
KeyboardEvent,
} from 'react';
import classNames from 'classnames';

import { CommonProps } from '../common';
import { CommonProps, ExclusiveUnion } from '../common';
import {
EuiFormRow,
EuiFormRowProps,
Expand All @@ -42,7 +43,6 @@ export type EuiInlineEditCommonProps = Omit<
'children'
> &
CommonProps & {
defaultValue: string;
placeholder?: string;
/**
* Callback that fires when a user clicks the save button.
Expand Down Expand Up @@ -90,7 +90,23 @@ export type EuiInlineEditCommonProps = Omit<
* Locks inline edit in read mode and displays the text value
*/
isReadOnly?: boolean;
};
} & ExclusiveUnion<
{
/**
* Initial inline edit text value
*/
defaultValue: string;
},
{
/**
* To use inline edit as a controlled component, continuously pass the value via this prop
*/
value: string;
// TODO: Update to an HTML Change Event
onChange: (event: any) => void;
onCancel: (perviousValue: string) => void;
}
>;

// Internal-only props, passed by the consumer-facing components
export type EuiInlineEditFormProps = EuiInlineEditCommonProps & {
Expand Down Expand Up @@ -125,6 +141,9 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
children,
sizes,
defaultValue,
value,
onChange,
onCancel,
placeholder,
inputAriaLabel,
startWithEditOpen,
Expand Down Expand Up @@ -168,8 +187,16 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
]);

const [isEditing, setIsEditing] = useState(false || startWithEditOpen);
const [editModeValue, setEditModeValue] = useState(defaultValue);
const [readModeValue, setReadModeValue] = useState(defaultValue);
const [editModeValue, setEditModeValue] = useState(value || defaultValue);
const [readModeValue, setReadModeValue] = useState(value || defaultValue);
Comment on lines +190 to +191

@cee-chen cee-chen Aug 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My 2c is I don't think we need to use value || here for our internal state. Since they're only used for uncontrolled behavior, they only need to listen for defaultValue.

Suggested change
const [editModeValue, setEditModeValue] = useState(value || defaultValue);
const [readModeValue, setReadModeValue] = useState(value || defaultValue);
const [editModeValue, setEditModeValue] = useState(defaultValue);
const [readModeValue, setReadModeValue] = useState(defaultValue);

@breehall breehall Aug 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This logic is actually how I'm able to pass readModeValue to the onCancel function. Otherwise, the readModeValue falls out of sync and isn't a reliable backup of the previous value.
Link to code sample mentioned: #7117 (comment)

@cee-chen cee-chen Aug 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh super interesting! If readModeValue is the only one that needs this logic, let's set it just for that and add an inline comment noting why


const valueToUse = useMemo(() => {
if (value) {
Comment on lines +193 to +194

@cee-chen cee-chen Aug 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[naming things is hard] how do you feel about renaming the consumer-passed value prop to value: controlledValue or possibly just _value, and then just naming this value?

return value;
} else {
return isEditing ? editModeValue : readModeValue || placeholder;

@cee-chen cee-chen Aug 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still can't get over how elegant this ternary is haha. Super awesome!

}
}, [value, editModeValue, readModeValue, isEditing, placeholder]);

const readModeStyles = euiInlineEditReadModeStyles(euiTheme);
const readModeCssStyles = [
Expand All @@ -186,20 +213,21 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({

const cancelInlineEdit = () => {
setEditModeValue(readModeValue);
onCancel && onCancel(readModeValue);

@breehall breehall Aug 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am using readModeValue instead of valueToUse here because is contains the prior text value

setIsEditing(false);
requestAnimationFrame(() => readModeFocusRef.current?.focus());
};

const saveInlineEditValue = async () => {
// If an onSave callback is present, and returns false, stay in edit mode
if (onSave) {
const onSaveReturn = onSave(editModeValue);
const onSaveReturn = onSave(valueToUse);

@cee-chen cee-chen Aug 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We'll probably also want to test that onSave works with a controlled value as well. How you prefer to test that is up to you; whether via docs, Jest test, or Storybook

const awaitedReturn =
onSaveReturn instanceof Promise ? await onSaveReturn : onSaveReturn;
if (awaitedReturn === false) return;
}

setReadModeValue(editModeValue);
setReadModeValue(valueToUse);
setIsEditing(false);
requestAnimationFrame(() => readModeFocusRef.current?.focus());
};
Expand Down Expand Up @@ -234,7 +262,7 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
>
<EuiFieldText
fullWidth
value={editModeValue}
value={valueToUse}
aria-label={inputAriaLabel}
compressed={sizes.compressed}
isInvalid={isInvalid}
Expand All @@ -245,6 +273,7 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
inputRef={setEditModeRefs}
onChange={(e) => {
setEditModeValue(e.target.value);
onChange && onChange(e);

@cee-chen cee-chen Aug 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

quick nit while I'm here: onChange?.(e) for brevity

editModeProps?.inputProps?.onChange?.(e);
}}
onKeyDown={(e) => {
Expand Down Expand Up @@ -333,7 +362,7 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
data-test-subj="euiInlineReadModeButton"
disabled={isReadOnly}
css={readModeCssStyles}
title={readModeValue || placeholder}
title={valueToUse}
{...readModeProps}
buttonRef={setReadModeRefs}
aria-describedby={classNames(
Expand All @@ -345,7 +374,7 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
readModeProps?.onClick?.(e);
}}
>
{children(readModeValue || placeholder)}
{children(valueToUse)}
</EuiButtonEmpty>
<span id={readModeDescribedById} hidden>
{!isReadOnly && (
Expand Down