feat: add inline-edit package #938
Conversation
🦋 Changeset detectedLatest commit: 9aaa2c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| }, | ||
| }); | ||
|
|
||
| type InlineVariants = RecipeVariants<typeof inline>; |
There was a problem hiding this comment.
This Vanilla Extract utility returns type layout: "vertical" | "horizontal" for the prop which means we can have a single source of truth for props tied directly to component styles (variants).
|
Size Change: +38 B (0%) Total Size: 161 kB
ℹ️ View Unchanged
|
.changeset/chatty-tables-attend.md
Outdated
|
|
||
| Add `inline-edit` package to display and allow inline editing of a form elements: | ||
|
|
||
| - Use props `defaultValue` and `onSave` to handle state management of the value to edit |
There was a problem hiding this comment.
This means this component cannot be "controlled", right?
Are there use cases where we might want it to be controlled too?
There was a problem hiding this comment.
Yeah having controlled can be useful! Will add it.
.changeset/chatty-tables-attend.md
Outdated
| - Have children act as the "read" view of the component | ||
| - Hide edit icon button and wrap children with a React Aria button when `hideEdit` is true | ||
| - Implement focus management to ensure focus is directed correctly when toggling between read and edit mode | ||
| - Use `input` prop to allow passing a custom `TextField` or `TextArea` component |
There was a problem hiding this comment.
Would naming this renderInput make this self-explanatory?
| input = <TextField />, | ||
| 'aria-label': ariaLabel, | ||
| }: InlineEditProps) => { | ||
| const [isEditing, setEditing] = useState(false); |
There was a problem hiding this comment.
Same question the one about defaultValue and un/controlled.
Are there use cases where it would be useful to pass isEditing from the owner component?
| setEditing(false); | ||
| }} | ||
| > | ||
| <span>{editValue}</span> |
There was a problem hiding this comment.
Did you get a chance to try using a render function for children? The idea was that in uncontrolled mode, it would remove the need for consumers to manage the state of the field's value.
There was a problem hiding this comment.
I did but was struggling with the DX. Do you have an idea what something like that would look like? Seems like folks would still need to use onConfirm to have access to the updated value to do something with it form/data wise.
| !controlled && setEditing(false); | ||
| }; | ||
|
|
||
| const handleKeyDown: KeyboardEventHandler<HTMLInputElement> = (event) => { |
There was a problem hiding this comment.
Was exploring the new stories in this branch locally, noticed that pressing Esc will cancel an in-progress edit which brought me here, wondering if we should also consider the case where a user blurs away from the input/cancel/save buttons while editing (either by clicking outside of the affected elements, or via keyboard navigation)?
There is some nuance to consider, but I'm wondering about the case where multiple inline-editable fields exist next to each other... how would a user navigate between them? For example: if I begin editing a field but then try to keyboard navigate to another editable field, what should happen? I'd expect the edit for the first field to be implicitly cancelled, but curious to hear your thoughts.
There was a problem hiding this comment.
Yeah no strong opinion on blur implicitly cancelling. Can try that out and see how that feels.
There was a problem hiding this comment.
What are the downsides of leaving the inline edits in editing mode when tabbing around?
The escape has less ambiguous intent than tabbing around.
One way to think about this maybe: do we want this component to be modal, where interacting with another component would affect the state of this component? (Think popover: once it's open, if the user interacts with anything else on the page, it should close.)
There was a problem hiding this comment.
One question is predictability: by making the interaction modal we can create the affordance that the edit is only persisted if the user explicitly chooses to do so (by clicking the save button, or pressing Enter)... anything else causes the draft to be discarded. This question also has implications for when we might consider a form "dirty" (which overlaps with routing behavior).
In the absence of a modal interaction, it's hard to reason about why we'd choose inline edit at all... if a field that I begin editing can be left in the editing state to move on to another field, what value is inline-edit adding over plain inputs?
Also to be clear I think this is very much a question that requires design feedback, I had just started to think about this while implementing elsewhere so the questions are fresh in mind.
There was a problem hiding this comment.
what value is inline-edit adding over plain inputs?
Yeah. There are places where we use an inline edit to make a field opt-in, like flag keys upon creation.
This question also has implications for when we might consider a form "dirty" (which overlaps with routing behavior).
How does this overlap with routing?
If the user hasn't confirmed their input, I'd expect form's state to be unchanged.
Anyways, these are good points for UX crit. I don't think these are that complicated, so long as we're consistent.
There was a problem hiding this comment.
How does this overlap with routing?
Today we consider a form dirty if the input hasn't been saved, and dirty forms (in most places) cause route changes to be blocked and the user is forced to confirm the navigation... if we make this interaction modal, blurring a field that is being edited would discard the draft (and therefore not cause the field to be considered dirty).
Conversely, if this interaction isn't modal the following scenario seems possible: begin editing a field, change the field's value, then navigate to a different field... in that case the first field would need to be considered dirty to prevent unexpectedly losing edits.
I also agree that these questions aren't especially complicated, and that we don't necessarily have to answer them in this PR.
There was a problem hiding this comment.
That said, we should be opinionated here before we show up at UX crit.
My 2¢: Let's cancel on blur for now. That feels snappier to me. 😅
There was a problem hiding this comment.
Meeting with Antonio shortly and can go over this topic.
There was a problem hiding this comment.
Discussed with Antonio and we are gonna go with cancel on blur 👍
|
Any thoughts in making the entire component interactive when in non-editable mode? Clicking could enable editable mode. That would give us a larger click area, and bring us closer to the native contenteditable behavior. We could also give some subtle treatment to that interaction to make that clear. |
Summary
Add
inline-editpackage to display and allow inline editing of a form elements:defaultValueandonConfirmto handle state management of the value to edithideEditis truerenderInputprop to allow passing a customTextFieldorTextAreacomponentisEditingto allow full control over the read and edit modes@vanilla-extract/cssas a peer dependency for proplayoutvariant typesuseFocusWithinto cancel edit on blurTesting approaches
https://626696a2018c1f004a1cde86-biuafqdqpl.chromatic.com/