EuiDescribedFormGroup component #707
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| import React, { | ||
| Component, | ||
| } from 'react'; | ||
|
|
||
| import { | ||
| EuiButton, | ||
| EuiCheckboxGroup, | ||
| EuiCode, | ||
| EuiFieldText, | ||
| EuiForm, | ||
| EuiFormRow, | ||
| EuiDescriptiveFormRow, | ||
|
There was a problem hiding this comment. Is the name of this component There was a problem hiding this comment. renamed to |
||
| EuiFilePicker, | ||
| EuiRange, | ||
| EuiSelect, | ||
| EuiSwitch, | ||
| } from '../../../../src/components'; | ||
|
|
||
| import makeId from '../../../../src/components/form/form_row/make_id'; | ||
|
|
||
| export default class extends Component { | ||
| constructor(props) { | ||
| super(props); | ||
|
|
||
| const idPrefix = makeId(); | ||
|
|
||
| this.state = { | ||
| isSwitchChecked: false, | ||
| checkboxes: [{ | ||
| id: `${idPrefix}0`, | ||
| label: 'Option one', | ||
| }, { | ||
| id: `${idPrefix}1`, | ||
| label: 'Option two is checked by default', | ||
| }, { | ||
| id: `${idPrefix}2`, | ||
| label: 'Option three', | ||
| }], | ||
| checkboxIdToSelectedMap: { | ||
| [`${idPrefix}1`]: true, | ||
| }, | ||
| radios: [{ | ||
| id: `${idPrefix}4`, | ||
| label: 'Option one', | ||
| }, { | ||
| id: `${idPrefix}5`, | ||
| label: 'Option two is selected by default', | ||
| }, { | ||
| id: `${idPrefix}6`, | ||
| label: 'Option three', | ||
| }], | ||
| radioIdSelected: `${idPrefix}5`, | ||
| }; | ||
| } | ||
|
|
||
| onSwitchChange = () => { | ||
| this.setState({ | ||
| isSwitchChecked: !this.state.isSwitchChecked, | ||
| }); | ||
| } | ||
|
|
||
| onCheckboxChange = optionId => { | ||
| const newCheckboxIdToSelectedMap = ({ ...this.state.checkboxIdToSelectedMap, ...{ | ||
| [optionId]: !this.state.checkboxIdToSelectedMap[optionId], | ||
| } }); | ||
|
|
||
| this.setState({ | ||
| checkboxIdToSelectedMap: newCheckboxIdToSelectedMap, | ||
| }); | ||
| } | ||
|
|
||
| onRadioChange = optionId => { | ||
| this.setState({ | ||
| radioIdSelected: optionId, | ||
| }); | ||
| } | ||
|
|
||
| render() { | ||
| return ( | ||
| <EuiForm> | ||
| <EuiDescriptiveFormRow | ||
| id="single-example" | ||
| title={<h3>Single text field</h3>} | ||
| paddingSize="m" | ||
| description={ | ||
| <span> | ||
| When using this with a single field where the text here serves as the help text for the input, | ||
| it is a good idea to give it <EuiCode>someID</EuiCode> and pass <EuiCode>someID-description</EuiCode> to | ||
| the form row's <EuiCode>describedByIds</EuiCode> prop. | ||
| </span> | ||
| } | ||
| > | ||
| <EuiFormRow | ||
| label="Text field" | ||
| describedByIds={['single-example-description']} | ||
|
There was a problem hiding this comment. As a consumer, I'd have to know about the implementation details of EuiDescriptiveFormRow to know that I think the solution is to apply the ID directly to the EuiFlexItem component which encompasses the title and the description. This way the consumer can just reference the same ID they're passing in already. I tested this and it seems to work well. If we make this change, we'd have to update the documentation too. There was a problem hiding this comment. I changed it so that the user will pass in If they pass |
||
| > | ||
| <EuiFieldText name="first" /> | ||
| </EuiFormRow> | ||
| </EuiDescriptiveFormRow> | ||
|
|
||
| <EuiDescriptiveFormRow | ||
| title={<strong>Multiple fields</strong>} | ||
| paddingSize="s" | ||
| description="Here are three form rows. The first form row does not have a title." | ||
| > | ||
| <EuiFormRow | ||
| hasEmptyLabelSpace | ||
| helpText={ | ||
| <span> | ||
| We do not pass <EuiCode>describedByIds</EuiCode> when there are multiple form rows. | ||
| </span> | ||
| } | ||
| > | ||
| <EuiSelect | ||
| hasNoInitialSelection | ||
| options={[ | ||
| { value: 'option_one', text: 'Option one' }, | ||
| { value: 'option_two', text: 'Option two' }, | ||
| { value: 'option_three', text: 'Option three' }, | ||
| ]} | ||
| /> | ||
| </EuiFormRow> | ||
|
|
||
| <EuiFormRow | ||
| label="File picker" | ||
| > | ||
| <EuiFilePicker /> | ||
| </EuiFormRow> | ||
|
|
||
| <EuiFormRow | ||
| label="Range" | ||
| > | ||
| <EuiRange | ||
| min={0} | ||
| max={100} | ||
| name="range" | ||
| id="range" | ||
| /> | ||
| </EuiFormRow> | ||
| </EuiDescriptiveFormRow> | ||
|
|
||
| <EuiDescriptiveFormRow | ||
| title={<h2>Full width</h2>} | ||
| paddingSize="l" | ||
| description={ | ||
| <span> | ||
| By default, <EuiCode>EuiDescriptiveFormRow</EuiCode> will be double the default width of form elements. | ||
| However, you can pass <EuiCode>fullWidth</EuiCode> prop to this, the individual field and row components | ||
| to expand to their container. | ||
| </span> | ||
| } | ||
| fullWidth | ||
| > | ||
| <EuiFormRow | ||
| label="Use a switch instead of a single checkbox" | ||
| fullWidth | ||
| > | ||
| <EuiSwitch | ||
| name="switch" | ||
| label="Should we do this?" | ||
| checked={this.state.isSwitchChecked} | ||
| onChange={this.onSwitchChange} | ||
| fullWidth | ||
| /> | ||
| </EuiFormRow> | ||
|
|
||
| <EuiFormRow> | ||
| <EuiFieldText name="second" /> | ||
| </EuiFormRow> | ||
|
|
||
| <EuiFormRow | ||
| label="Checkboxes" | ||
| fullWidth | ||
| > | ||
| <EuiCheckboxGroup | ||
| options={this.state.checkboxes} | ||
| idToSelectedMap={this.state.checkboxIdToSelectedMap} | ||
| onChange={this.onCheckboxChange} | ||
| fullWidth | ||
| /> | ||
| </EuiFormRow> | ||
| </EuiDescriptiveFormRow> | ||
|
|
||
| <EuiButton type="submit" fill> | ||
| Save form | ||
| </EuiButton> | ||
| </EuiForm> | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,6 @@ export class EuiFormRow extends Component { | |
| const onChildFocus = get(this.props, 'children.props.onFocus'); | ||
| if (onChildFocus) { | ||
| onChildFocus(...args); | ||
| } | ||
|
|
||
| this.setState({ | ||
|
|
@@ -60,6 +59,7 @@ export class EuiFormRow extends Component { | |
| hasEmptyLabelSpace, | ||
| fullWidth, | ||
| className, | ||
| describedByIds, | ||
|
There was a problem hiding this comment. I think you can remove all these There was a problem hiding this comment. TBH I think this is a pretty nice improvement because it allows people to associate form row fields with whatever other help text they want. I think it's OK to leave it in, but I would definitely add another bullet to the CHANGELOG mentioning the addition of this prop, and document it with an example in the docs site. There was a problem hiding this comment. That's fine. We can keep it in but we should have @aphelionz check it to make sure it doesn't duplicate the reading of the title and description. There was a problem hiding this comment. Good call, I'd love to hear @aphelionz's thoughts on this. I tested it in VoiceOver on OS X and this does repeat the title and description, but I think that's just what happens when you associate elements with other elements using There was a problem hiding this comment. Can you add There was a problem hiding this comment. Sorry, to clarify the There was a problem hiding this comment. I've been staring at the WCAG docs trying to figure out which criterion this would fall under.... is the text required to be understood succinctly by a user before requiring input? Then it's a level A... however if it's not it falls to level AA. Level A: https://www.w3.org/WAI/WCAG20/quickref/#minimize-error-cues vs Level AA: https://www.w3.org/WAI/WCAG20/quickref/#qr-navigation-mechanisms-descriptive There was a problem hiding this comment. Maybe we should ensure that users use heading tags for the title by doing: There was a problem hiding this comment. I think @jen-huang's latest changes are sound and result in a good-enough UX. Maybe we can come back and reassess accessibility later on? There was a problem hiding this comment. I adjusted the aria and IDing a bit with the last update, but I think more consideration is needed for handling accessibility. I created #762 for more work 🙂 |
||
| ...rest | ||
| } = this.props; | ||
|
|
||
|
|
@@ -109,7 +109,7 @@ export class EuiFormRow extends Component { | |
| ); | ||
| } | ||
|
|
||
| const describingIds = [...describedByIds]; | ||
| if (optionalHelpText) { | ||
| describingIds.push(optionalHelpText.props.id); | ||
| } | ||
|
|
@@ -154,9 +154,14 @@ EuiFormRow.propTypes = { | |
| helpText: PropTypes.node, | ||
| hasEmptyLabelSpace: PropTypes.bool, | ||
| fullWidth: PropTypes.bool, | ||
| /** | ||
| * IDs of additional elements that should be part of children's `aria-describedby` | ||
|
There was a problem hiding this comment. 👍 Thanks for the descriptions! |
||
| */ | ||
| describedByIds: PropTypes.array, | ||
| }; | ||
|
|
||
| EuiFormRow.defaultProps = { | ||
| hasEmptyLabelSpace: false, | ||
| fullWidth: false, | ||
| describedByIds: [], | ||
| }; | ||
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.
Small typo: EuiFormRows -> EuiFormRow
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.
fixed