-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiInlineEdit] Create placeholder
Prop
#6872
Conversation
…This prop adds a different style in readMode and gives the editMode form control the placeholder prop
…holder prop. Update required snapshots
|
||
hasPlaceholder: css` | ||
&.euiButtonEmpty .euiText, | ||
&.euiButtonEmpty .euiTitle { | ||
font-style: italic; | ||
color: ${euiTheme.colors.subduedText}; | ||
} | ||
`, |
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.
The nested styling is mostly required for EuiInlineEditTitle
because the text is not a direct descendant of the readMode
button.
I chose to add it here because InlineEditForm
is handling most of the logic around renderPlaceholder
.
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 think that's a legit reason for the title/text nesting, but is the extra &.euiButtonEmpty
selector necessary or can we remove it?
hasPlaceholder: css` | |
&.euiButtonEmpty .euiText, | |
&.euiButtonEmpty .euiTitle { | |
font-style: italic; | |
color: ${euiTheme.colors.subduedText}; | |
} | |
`, | |
hasPlaceholder: css` | |
.euiText, | |
.euiTitle { | |
font-style: italic; | |
color: ${euiTheme.colors.subduedText}; | |
} | |
`, |
const [editModeValue, setEditModeValue] = useState( | ||
renderPlaceholder ? '' : defaultValue | ||
); |
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.
When renderPlaceholder
is true, we don't want to add a value to the editMode
input so the placeholder text can be seen.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6872/ |
/** | ||
* Adds styling to display defaultValue as a placeholder in `readMode`. | ||
* Adds defaultValue as a placeholder on the `editMode` form control and does not populate the value. | ||
*/ | ||
displayDefaultValueAsPlaceholder?: boolean; |
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'm not really a huge fan of this API. Why wouldn't we instead accept a placeholder string that displays whenever defaultValue
is ''
? If consumers want to start with it, they can do, e.g.
<EuiInlineEditText
placeholder="Click here to edit text in place"
defaultValue=""
/>
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.
@cee-chen The reason I decided to do it this way is because both defaultValue
and placeholder
are serve as starting values for the component. This is the approach I originally took, but I think it could cause confusion if both defaultValue
and placeholder
are both passed in as in your example above. My next thought was to restrict the props with TypeScript (i.e you can pass placeholder
or defaultValue
, but not both).
Compared to those options, I actually prefer this instead. There's one string to set and the consumer can determine how it's styles with a boolean.
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.
both
defaultValue
andplaceholder
are serve as starting values for the component
That's not quite correct or how placeholder
works in inputs. It's not specifically about the starting value, it's about the empty value. Placeholders reappear in inputs when inputs are cleared or set to ''
. The placeholder text should reappear if the user adds text but then later decides to clear it.
I feel fairly strongly we should try to follow existing input/field patterns as natively as is possible for non-native behavior. This shouldn't be confusing to most developers that frequently work with native inputs to be frank, I would consider this API to be far more confusing.
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.
Right, this is my understanding for inputs. But for the purpose of adding the placeholder to inline edit, that wouldn't be any different than the consumer using editMode.inputProps.placeholder
, right?
The goal is to provide a distinction in readMode
that the value displayed is temporary and not final (until the first official save). If the placeholder
prop is present at all times, I think we would need another prop to give consumers the option to have that "unofficial" styling on the readMode
button. We would need a way to know if we should show the text as default
or placeholder
.
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.
What about something like:
<EuiInlineEditText
placeholder="Click here to edit text in place"
defaultValue="A different value"
readModeDisplay = "placeholder" | "defaultValue"
/>
The readModeDisplay could be used to provide that distinct styling (added in this PR). And it would determine which text value should actually be shown in readMode
. This does mean if both fields are passed, the placeholder
in editMode
won't actually be visible unless the users clears the text input.
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.
Ok, let me spell it all out to make sure my understanding is correct:
Proposed API:
<EuiInlineEditText
placeholder="Click here to edit text in place"
defaultValue=""
/>
defaultValue
will remain a required prop that will accept an empty string
placeholder
will be an optional prop that can be passed with no restrictions
In readMode
:
-
placeholder
will show as the value in the button if there's a placeholder, but nodefaultValue
-
defaultValue
will show as the value in the button if:- both
placeholder
andvalue
are passed in OR - just
defaultValue
is passed in (with noplaceholder
)
- both
In editMode
:
-
placeholder
will be set to the as theplaceholder
on the input form control -
defaultValue
will continue to be set to thevalue
on the form control
Is this accurate @cee-chen ?
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.
That looks great to me! The basic condition to display it in read mode is show the placeholder if it exists and if defaultValue is ''/falsy
.
A few quick minor nuances/clarifications:
placeholder will be an optional prop that can be passed with no restrictions
placeholder
should be an option prop with a string
type (i.e. it should not allow any React JSX or dom)
placeholder will be set to the as the placeholder on the input form control
For maximum flexibility, we should allow consumers to override this via editModeProps.inputProps.placeholder
(which allows consumers to set separate placeholders for read mode vs edit mode if they absolutely have to. In most cases they shouldn't/won't, but I just know someone will ask for that someday 🥲 )
This should be as simple as making sure the spread operator comes after the placeholder prop declaration, i.e.
<EuiFieldText
placeholder={placeholder}
{...editModeProps?.inputProps}
/>
LMK if you have any other questions!
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.
@cee-chen As I'm updating tests, I just thought about this comment. With the API described above, consumers can still save empty strings in editMode
. The above mostly accounts for the component state at load. I just want to be sure of a few this before reopening the PR.
Here are a few scenarios where an empty string being saved would result in a "blank" read mode.
defaultValue |
placeholder |
empty string saved | readMode result |
---|---|---|---|
Present | undefined |
➡️ | Empty string in readMode showing only the pencil icon |
'' |
Present | ➡️ | Empty string in readMode showing only the pencil icon |
'' |
undefined |
➡️ | Empty string in readMode showing only the pencil icon |
The placeholder is a blanket at load, but if consumers don't provide validation to prevent saving empty strings, they'll likely come across a "blank" read mode.
Is this too opinionated? It would eliminate the second case in the table above.
const saveInlineEditValue = async () => {
...
showPlaceholder && editModeValue === '' ? setEditModeValue(placeholder) : setEditModeValue(readModeValue);
...
};
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.
The placeholder is a blanket at load, but if consumers don't provide validation to prevent saving empty strings, they'll likely come across a "blank" read mode.
This shouldn't be happening. What logic are you using to show the placeholder in readMode? It should be based on a stateful value, not on defaultValue
. There is very little we should be doing with defaultValue
other than passing it in to initial state.
You likely want this: const showPlaceholder = placeholder && !readModeValue;
With the above, the placeholder will show even if the user saves an empty string and the consumer doesn't have any validation in place (which is also exactly how native inputs behave as well).
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'm glad I asked because that's a little different than what I described a few comments up. Let me push my commit so I can show you. Maybe if you have time tomorrow, we can pair review it?
test('displayDefaultValueAsPlaceholder', () => { | ||
const { container } = render( | ||
<EuiInlineEditTitle | ||
displayDefaultValueAsPlaceholder={true} | ||
{...inlineEditTitleProps} | ||
/> | ||
); | ||
|
||
expect(container.firstChild).toMatchSnapshot(); | ||
}); | ||
|
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.
inline_edit_title
doesn't need a test for this prop since it's a basic passthrough to inline_edit_form
. The majority of the testing should be done there
test('displayDefaultValueAsPlaceholder', () => { | |
const { container } = render( | |
<EuiInlineEditTitle | |
displayDefaultValueAsPlaceholder={true} | |
{...inlineEditTitleProps} | |
/> | |
); | |
expect(container.firstChild).toMatchSnapshot(); | |
}); |
This one concerns me a bit. Particularly the possibility for an empty string. I'm not sure of the solution yet, but I think we need to force a string to be present. Again, I don't quite know what this means, as I know this goes against the norm for typical inputs. I'll think more on it and send along some thoughts, but wanted to raise a flag. |
@mdefazio This functionality isn't actually being added as part of this PR. All variations of inline edit have the potential to be saved with no text. There was an "empty string check" originally, but in this PR , it was removed. I believe the thought was the API was a little too opinionated and we provide consumers with the validation options so they can have full control over it. Should we revisit this decision? |
But that's what a |
This reverts commit 49bbebe.
displayDefaultValueAsPlaceholder
Propplaceholder
Prop
…' prop. This prop adds a different style in readMode and gives the editMode form control the placeholder prop" This reverts commit 4bda833. Revert changes that previously added the displayDefaultValueAsPlaceholder prop
Preview documentation changes for this PR: https://eui.elastic.co/pr_6872/ |
Created PR #6883 to handle to take over this task. Closing this PR and moving forward with that one. |
Fixes #6857
Summary
Creation of the
displayDefaultValueAsPlaceholder
prop forEuiInlineEdit
. This is boolean that determines if the string provided bydefaultValue
should display as a placeholder or not.In
readMode
, the style of the text is updated to a lighter color with italic font. IneditMode
, the form control is passed a placeholder and does not contain the default value.If a placeholder is set, it will last until text is successfully saved by inline edit. At that point, the placeholder styling and input element placeholder are both removed.
QA
Head over to
EuiInlineEdit
in staging and ensure you can do the following:readMode
Remove or strikethrough items that do not apply to your PR.
General checklist
@default
if default values are missing) and playground toggles