[EuiInlineEdit] (POC Review) Create Component Directory and Base Functionality #6533
Conversation
…ase component that accepts the EuiText, EuiTextarea, and EuiComboBox controls and provides both read and edit modes.
…it. WithDefaultProps checks to ensure that the editViewTypeProps matches the editViewType being passed in. For exmaple, if the editViewType is EuiFieldText, WithDefaultProps ensure that editViewTypeProps matches the shape of the props for EuiFieldText.
…ontrols from being saved when there is no text or combo box selections.
…when a save was attempted with no selections
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6533/ |
breehall
changed the title
| | typeof EuiTextArea | ||
| | typeof EuiFieldText | ||
| | typeof EuiComboBox; |
There was a problem hiding this comment.
Quick FYI - typeof will get unspeakably spicy/gnarly if any of these components are class components that need to be wrapped in the withEuiTheme HOC during Emotion conversions.
I'm particularly leery of EuiComboBox (complex class component, unlikely to be converted to function). Do we know for sure that inline editing badges is necessary / desired by any specific team? Maybe I'm just confused by why UX-wise, a designer wouldn't use a EuiComboBox in the first place? 🤔
There was a problem hiding this comment.
I'm particularly leery of EuiComboBox
So am I and it was actually one of the most tricky pieces of this so far. Adding in ComboBox was a "blue sky" idea that came directly from the Atlassian design system as inspo. I never got a real use case for where this would be useful. I talked with Jason not too long ago about moving forward with the text options for now if there's no practical need for it at this very moment.
There was a problem hiding this comment.
I would strongly strongly push for shipping initially without EuiComboBox and just text/textarea/number to start. I'm not seeing anything in the original GH issue/discussion or the spec doc that shows selection anywhere, so I really don't think this needs to be something we try to support without a strong use case or team asking for it.
There was a problem hiding this comment.
Quick FYI - typeof will get unspeakably spicy/gnarly if any of these components are class components that need to be wrapped in the withEuiTheme HOC during Emotion conversions.
By chance, do we have another component I can use as a guide for this? Or do we have a pattern in place that I can reference instead of using typeof?
There was a problem hiding this comment.
EuiDualRange and EuiAccordion are two class components wrapped with withEuiTheme that you can try using off the top of my head. Here's my most recent comment with more context as to how we know the typing starts getting Very Bad: #6539 (comment)
| /** | ||
| * Props for the form control created by editViewType | ||
| */ | ||
| editViewTypeProps?: WithDefaultPropsApplied<T>; |
There was a problem hiding this comment.
I'm struggling to understand the type usecase of Defaultize<> here - is there any specific reason why React.ComponentProps<T> doesn't just work as-is? We're not really expecting consumers to pass in any props that are defaults, just props to override defaults, right?
| </> | ||
| ); | ||
|
|
||
| const comboBoxReadViewElement = ( |
There was a problem hiding this comment.
[attaching to a semi random line for threading]
At first glance through this file, the code feels elegant and easy to read/understand for just the text implementation. Adding the combobox into the mix makes it significantly harder to follow. If we're going to move ahead with including EuiComboBox, I would strongly suggest making it its own separate component, e.g. InlineEditSelect instead to help make code less convoluted/branching.
| id={inlineTextEditInputId} | ||
| value={textEditViewValue} | ||
| onChange={editTextViewOnChange} | ||
| {...(rest as any)} |
There was a problem hiding this comment.
UX/accessibility note - can we try adding an autoFocus to the props here so that keyboard focus automatically transfers into the editable field?
There was a problem hiding this comment.
+1. autoFocus for the input will cause screen readers to announce the accessible label immediately, and help users understand what the button click changed in UI.
There was a problem hiding this comment.
I'd also be in favor of setting focus back on the button when users confirm or cancel their edit. That's taking a page out of the modal dialog playbook, but feels like the right UX for keyboard and screen reader navigation.
| //@ts-ignore TypeScript sad :( | ||
| editViewType = EuiFieldText, | ||
| editViewTypeProps, |
There was a problem hiding this comment.
[just writing this out as a stream of consciousness thought, this is not a change request]
Just curious - if we do want to make the decision to make this a more restricted component in terms of types of inputs/fields allowed, why not do something like this:
type: 'text' | 'textarea' | 'number', // use a switch statement to determine input based on this type
inputProps, // whatever is passed to the input/field component
I kind of see the goal here of making this a very 'extensible' component that allows potentially wildcard kinds of inputs/fields... but I don't fully see the advantage of someone being able to (e.g.) pass a raw <input> - EUI is already an opinionated design system with styling OOTB, not a lower level library 🤷
FWIW, the main disadvantage of my proposal is that conditional inputProps typing based on the type prop becomes a bit of a headache / starts getting into ExclusiveUnion land 😅
There was a problem hiding this comment.
After having written that all out, I actually think I've managed to convince myself against my own train of thought. I could go either way, but I do like the current approach that gives consumers the ability to pass in whatever insane component they want as a more permissive escape hatch. Leaving this thread up though in case other folks have more thoughts!
Quick temperature check from the rest of the team. Would it simplify things to limit this just to text, and drop support for a combo box? At least, for a first iteration of this component? |
|
It's looking good @breehall! I agree with @cee-chen
I think we should just focus on EuiInlineEdit Text and TextArea for now. I created a new Figma project for the design, it is still a work in progress. I will need to go through some flows to see how the component will work: https://www.figma.com/file/v9DYJMnt7ynEycyR8iYfhz/EuiInlineEdit?t=ZHot1OojvzVOc9hm-6. But I have some initial questions related with the props. It accepts a
Also one thing that concerns me on the current design is that there is no clue that the text is editable. For example, for the same example from Observability -> Cases let's say I didn't have that
On the following example from enterprise search, let's say I would click the edit button and I would only want to allow inline edit the
I will continue going trough the design and explore some user flows. I will also talk with @MichaelMarcialis to try to understand his use cases for this component (#3928 (comment)). Then we can have a zoom call and go through the flows. |
There was a problem hiding this comment.
This is going to be an excellent addition to EUI @breehall! I left some first-impression a11y comments, trying to offer guidance than be prescriptive. Feel free to holler at me next week to keep testing as you add to the PR, or to discuss any feedback. Much appreciated!
| id={inlineTextEditInputId} | ||
| value={textEditViewValue} | ||
| onChange={editTextViewOnChange} | ||
| {...(rest as any)} |
There was a problem hiding this comment.
+1. autoFocus for the input will cause screen readers to announce the accessible label immediately, and help users understand what the button click changed in UI.
| <h3>EuiInlineEdit - Text</h3> | ||
| <EuiInlineEdit | ||
| editViewType={EuiFieldText} |
There was a problem hiding this comment.
When I started reviewing these with VoiceOver, I noticed it was reading back the input (or textarea) text but not the heading I presumed was the visual / accessible label. I think you could go a couple of directions here:
- Add documentation encouraging users to set unique IDs on each heading and pass that ID into an
aria-labelledbyprop in theEuiInlineEditwhen it's in edit state. - Make
labela required prop so it adds a visual and accessible label to the input. This gets in the way of the heading text in your examples. - Set a required
aria-labelprop on theEuiInlineEditcomponent that gets passed to the input or textarea. This is my least favorite option.
This change feels like it could affect visual and structural order, so happy to discuss it more next week and look for a good working solution.
| <EuiButtonIcon | ||
| iconType="check" | ||
| aria-label={confirmButtonAriaLabel || 'confirm'} | ||
| onClick={ | ||
| EditViewType === EuiComboBox | ||
| ? saveComboBoxEditValue | ||
| : saveTextEditValue | ||
| } // this should be a conditional to switch between text and combobox saves | ||
| /> | ||
| <EuiButtonIcon |
There was a problem hiding this comment.
These are good aria-label suggestions with fallbacks. I'm considering if we can build on the announcement by adding an aria-describedby that references either a label ID or a heading ID (see my previous comment about the accessible labels when in isInEdit state.
This would cause the button to read out "Confirm, button. pause EuiInlineEdit - Text". It adds a level of reinforcement what the user is editing or canceling. Not as necessary or useful on a text input, but the benefit increases greatly when users are editing a textarea and it's potentially been a minute since they heard what the label / group name was.
| id={inlineTextEditInputId} | ||
| value={textEditViewValue} | ||
| onChange={editTextViewOnChange} | ||
| {...(rest as any)} |
There was a problem hiding this comment.
I'd also be in favor of setting focus back on the button when users confirm or cancel their edit. That's taking a page out of the modal dialog playbook, but feels like the right UX for keyboard and screen reader navigation.
✨ POC Review Action Plan ✨Spec Changes
Accessibility Updates
Display as a Title |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6533/ |
Relates to #3928
Note to Reviewer
Please feel free to fully mark this PR! I'd like to solidify the base of this component before adding in the code for props from my other branch. I'm open to any changes to the code and design for this. I'm purposely leaving this PR in draft for the first round of reviews because there will likely need to be changes before merging this into the feature branch.
Things I'm looking for:
editViewcomponents in the future, but I partially believe it could be a lot cleaner if I just focus on the three intended componentsreadView. Is this still OK?In your opinion, does a multiple select combo box make sense for this component? Should this be a single select combo box if anything?
Summary
(Associated with #3928)
Spec Doc: EuiInlineEdit Spec Doc
This is the first PR for the new
EuiInlineEditcomponent. This PR creates the required directories for the component and the component docs. TheEuiInlineEditcomponent accepts three form controls (EuiFieldText,EuiTextarea, andEuiComboBox) and allows users to toggle between two states to view and edit the controls.The base functionality for
EuiInlineEditincludes:readViewandeditViewfor the three accepted form controlsreadViewshould display anEuiEmptyButtonwith text (forEuiFieldTextandEuiTextarea) or badges (forEuiComboBox) with the saved or default text and selectionseditViewshould display the desired form controleditViewreadViewwith the text / selections prior to editingeditViewQA
EuiInlineEditFieldText and TextAreaClick on the
helloWorldmessage or the cupcake ipsum message, change the text inside of the text box, use the submit button to save results. The new message should persist.ED29CA17-C3BD-4D50-8A34-C175089B0E6D_2_0_a.mov
EuiInlineEditComboBoxClick on the badges to open an
EuiComboBox. Select different options with the dropdown to add them to your list. Use the submit button to save results. A badge for each option selected should appear.3BA244C8-9BB1-4862-96B2-611B2A6CB4C2_2_0_a.mov
Cancelling an action within
EuiInlineEditFieldText and TextAreaClick on the
helloWorldmessage or the cupcake ipsum message, change the text inside of the text box, use the cancel button. The new message should not persist.Saving new text within
EuiInlineEditComboBoxClick on the badges to open an
EuiComboBox. Select different options with the dropdown to add them to your list. Use the cancel button to save results. The badges and their values should not change.General checklist