[EuiForm, EuiCallout] Added focus state to error callout #4497

akashgp09 · 2021-02-06T15:45:22Z

Summary

This PR Fixes: #1854

~~Added ability to show error callout at the bottom of the form .~~
~~User can now pass below as an option to invalidCallout prop of EuiForm.~~

Focus state shifts to EuiCallout when there is any validation error in EuiForm.

Screenshot

< EuiForm isInvalid={showErrors} error={errors} invalidCallout="below" >

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
Added documentation
~~- [ ] Checked Code Sandbox works for the any docs examples~~
- [ ] Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2021-02-06T15:45:24Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

chandlerprall

Thanks for making this change! Code changes look good overall, but have a couple small requests to improve documentation & clean up the component code a little.

chandlerprall · 2021-02-08T18:51:07Z

src-docs/src/views/form_validation/form_validation_example.js

+          display the callout above or below by passing
+          <EuiCode>invalidCallout=&ldquo;above&ldquo;</EuiCode>(default) or
+          <EuiCode>invalidCallout=&ldquo;below&ldquo;</EuiCode> depending upon
+          your choice. Additionally you can also hide the callout by passing


Suggested change

display the callout above or below by passing

<EuiCode>invalidCallout=“above“</EuiCode>(default) or

<EuiCode>invalidCallout=“below“</EuiCode> depending upon

your choice. Additionally you can also hide the callout by passing

display the callout above or below the form by passing

<EuiCode>invalidCallout=“above“</EuiCode>(default) or

<EuiCode>invalidCallout=“below“</EuiCode>.

Additionally, you can hide the callout by passing

these changes may need to be reformatted with different line lengths for prettier's linting

chandlerprall · 2021-02-08T18:54:05Z

src/components/form/form.tsx

@@ -73,7 +73,7 @@ export const EuiForm: FunctionComponent<EuiFormProps> = ({

  let optionalErrorAlert;

-  if (isInvalid && invalidCallout === 'above') {
+  if (isInvalid && (invalidCallout === 'above' || invalidCallout === 'below')) {


Let's invert this to check for !none vs. above || below

chandlerprall · 2021-02-08T18:55:41Z

src/components/form/form.tsx

+  if (invalidCallout === 'above') {
+    return (
+      <Element className={classes} {...(rest as HTMLAttributes<HTMLElement>)}>
+        {optionalErrorAlert}
+        {children}
+      </Element>
+    );
+  } else {
+    return (
+      <Element className={classes} {...(rest as HTMLAttributes<HTMLElement>)}>
+        {children}
+        {optionalErrorAlert}
+      </Element>
+    );
+  }


It seems a little cleaner to avoid the wrapping if and conditionally render optionalErrorAlert instead,

Suggested change

if (invalidCallout === 'above') {

return (

<Element className={classes} {...(rest as HTMLAttributes<HTMLElement>)}>

{optionalErrorAlert}

{children}

</Element>

);

} else {

return (

<Element className={classes} {...(rest as HTMLAttributes<HTMLElement>)}>

{children}

{optionalErrorAlert}

</Element>

);

}

if (invalidCallout === 'above') {

return (

<Element className={classes} {...(rest as HTMLAttributes<HTMLElement>)}>

{optionalErrorAlert}

{children}

</Element>

);

} else {

return (

<Element className={classes} {...(rest as HTMLAttributes<HTMLElement>)}>

{invalidCallout === 'above' && optionalErrorAlert}

{children}

{invalidCallout === 'below' && optionalErrorAlert}

</Element>

chandlerprall · 2021-02-08T19:07:25Z

src-docs/src/views/form_validation/validation.js

        {button}
+        <EuiSpacer />


Let's make this example interactive to show the two positions, I've tested by adding the following at line 36 of validation.js and adding EuiButtonGroup to the list of component imports at the top of the file

const [calloutPosition, setCalloutPosition] = useState('above'); return ( <Fragment> <EuiButtonGroup legend="sets callout position in the form" options={[ { id: 'above', label: 'Show callout above form' }, { id: 'below', label: 'Show callout below form' }, ]} idSelected={calloutPosition} onChange={setCalloutPosition} /> <EuiSpacer /> <EuiForm ...

chandlerprall · 2021-02-09T17:41:03Z

@akashgp09 thanks for the additional changes! We just had a discussion around this & what was asked for in #1854 and are going to think about the request more and what the best implementation is. For transparency, these thoughts include:

shift focus to the validation errors when the box appears, better for screen reader support & can scroll it into view
in addition to above/below, is there a version of the component that returns the validation errors element, allowing it to be placed anywhere in the form
pattern familiarity & recognition - does this or related changes cause confusion for users who are familiar with the existing pattern of always displaying errors at the top

cchaos · 2021-02-10T16:45:37Z

@akashgp09 If you feel up to experimenting on this one, I think the most ideal UX would be to:

Always have the form error at the top (don't actually support 'bottom')
When the user hits the submit button and there are errors, the user's focus moves to the EuiCallOut that is holding the errors. This accordion PR Shift focus to accordion content when expanded #4442 handled a very similar experience.
🤞 That by shifting the user's focus, the window should auto scroll (or put that focused item within the visible viewport), but if not, implement some sort of smooth scrollTo so that the callout is then visible.

akashgp09 · 2021-02-10T18:25:02Z

thank you for the suggestions, will make the changes soon :-)

akashgp09 · 2021-02-18T12:37:56Z

Added focus to Error

Before

Needed to scroll up to see the error

After

Auto scroll to the Error

chandlerprall · 2021-02-18T18:36:00Z

running CI to deploy this for testing: jenkins test this

kibanamachine · 2021-02-18T18:59:03Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4497/

cchaos · 2021-02-19T15:20:18Z

The auto-scroll works pretty well, but my guess is that the focused element has to include the title of the callout as well. So I'd lean towards putting the focus on the entire EuiCallOut instead of just the bullet list.

akashgp09 · 2021-02-19T16:58:00Z

will wrapping the EuiCallout component within a div work? this way it will cover the title of the callout as well

 <div
            tabIndex={-1}
            ref={(node) => {
              node && node.focus();
            }}>
            <EuiCallOut
              className="euiForm__errors"
              title={addressFormErrors}
              color="danger"
              role="alert"
              aria-live="assertive">
              {optionalErrors}
            </EuiCallOut>
</div>

akashgp09 · 2021-02-19T17:21:08Z

Or should i implement forwardRef to EuiCallout component to pass ref

cchaos · 2021-02-19T17:30:33Z

I would go the forwardRef route as it's most likely a common pattern to pulling attention to the EuiCallOut.

chandlerprall · 2021-02-19T18:17:13Z

src/components/form/form.tsx

@@ -84,6 +80,10 @@ export const EuiForm: FunctionComponent<EuiFormProps> = ({
        default="Please address the highlighted errors.">
        {(addressFormErrors: string) => (
          <EuiCallOut
+            tabIndex={-1}
+            ref={(node) => {


This is interesting. Because it creates the new ref function every render pass, it will re-focus the element every time and not only first mount. I imagine this will be aggravating in cases when the user tries to edit an invalid field, but we'll test what happens in practice.

So we need to focus the element only once, for the first mount ?
We can accomplish this by setting focus to the Element inside useEffect hook 🤔

Tested this by adding a setState to track the value of the example's textarea, and not only does the screen scroll back to the error box, but it takes focus off the textarea so I had to click back into it for each character. Either a useEffect, or moving the function passed to ref into a useCallback, should help clear that up.

chandlerprall · 2021-02-19T18:17:30Z

jenkins test this

kibanamachine · 2021-02-19T18:40:55Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4497/

akashgp09 · 2021-02-23T11:44:25Z

seems to be working fine, now the focus stays on the textarea while editing.

chandlerprall

Small syntax request & changelog update needs correction, but the code change & functionality looks good to me.

CHANGELOG.md

src/components/form/form.tsx

akashgp09 · 2021-02-26T16:49:51Z

@chandlerprall i have made the changes as suggested. Please let me know if there are any changes required.

CHANGELOG.md

cchaos · 2021-03-02T16:00:55Z

Jenkins, test this

kibanamachine · 2021-03-02T16:17:50Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4497/

cchaos · 2021-03-02T16:25:35Z

Jenkins, test this

cchaos

I checked this in Chrome, FF, Safari and Chrome mobile. I even checked how it would behave if the form existed in an element with overflow. They all seem to behave well. 🎉

kibanamachine · 2021-03-02T16:47:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4497/

chandlerprall

Changes LGTM; pulled & tested scenarios locally

cchaos · 2021-03-02T16:54:31Z

Jenkins, test this

kibanamachine · 2021-03-02T17:14:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4497/

akashgp09 · 2021-03-02T21:38:27Z

@chandlerprall @cchaos thank you for your continuous feedbacks and guidance on this one. Finally it's done ^_^

chandlerprall self-requested a review February 8, 2021 18:40

chandlerprall suggested changes Feb 8, 2021

View reviewed changes

cchaos self-requested a review February 8, 2021 19:59

akashgp09 requested a review from chandlerprall February 9, 2021 01:06

akashgp09 marked this pull request as draft February 14, 2021 16:20

akashgp09 added 4 commits February 14, 2021 22:13

show error callout in the bottom of the form

c2b8914

updated docs

b9db5cf

added EuiButtonGroup and other changes

d0b8c78

added focus on error

56755a0

akashgp09 force-pushed the EuiForm branch from 3534330 to 56755a0 Compare February 18, 2021 12:27

akashgp09 marked this pull request as ready for review February 18, 2021 12:27

updated

c018b23

akashgp09 changed the title ~~[EuiForm] Added ability to show error callout at the bottom of the form~~ [EuiForm] Added focus to error callout Feb 18, 2021

Merge branch 'master' into EuiForm

046689c

Implemented forwardRef route

e160e10

chandlerprall reviewed Feb 19, 2021

View reviewed changes

fixes

84c4ea4

chandlerprall suggested changes Feb 23, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/components/form/form.tsx Outdated Show resolved Hide resolved

akashgp09 added 2 commits February 24, 2021 17:53

clean up

69703ac

Merge branch 'master' into EuiForm

4519544

akashgp09 changed the title ~~[EuiForm] Added focus to error callout~~ [EuiForm, EuiCallout] Added focus state to error callout Feb 24, 2021

akashgp09 requested a review from chandlerprall February 24, 2021 13:00

Merge branch 'master' into EuiForm

22cebbe

cchaos reviewed Mar 2, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

24a9b84

Merge remote-tracking branch '2_upstream/master' into EuiForm

447d85f

cchaos approved these changes Mar 2, 2021

View reviewed changes

chandlerprall approved these changes Mar 2, 2021

View reviewed changes

Merge branch 'master' into EuiForm

2e8d6c2

cchaos merged commit e4e6970 into elastic:master Mar 2, 2021

chandlerprall mentioned this pull request Mar 5, 2021

Upgrade EUI to v31.10.0 elastic/kibana#93796

Merged

dependabot bot mentioned this pull request Mar 15, 2021

yarn upgrade: bump @elastic/eui from 31.8.0 to 31.10.0 Noxsios/config-composer#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiForm, EuiCallout] Added focus state to error callout #4497

[EuiForm, EuiCallout] Added focus state to error callout #4497

akashgp09 commented Feb 6, 2021 •

edited

Loading

kibanamachine commented Feb 6, 2021

chandlerprall left a comment

chandlerprall Feb 8, 2021

chandlerprall Feb 8, 2021

chandlerprall Feb 8, 2021

chandlerprall Feb 8, 2021

chandlerprall commented Feb 9, 2021

cchaos commented Feb 10, 2021

akashgp09 commented Feb 10, 2021

akashgp09 commented Feb 18, 2021

chandlerprall commented Feb 18, 2021

kibanamachine commented Feb 18, 2021

cchaos commented Feb 19, 2021 •

edited

Loading

akashgp09 commented Feb 19, 2021 •

edited

Loading

akashgp09 commented Feb 19, 2021

cchaos commented Feb 19, 2021

chandlerprall Feb 19, 2021

akashgp09 Feb 22, 2021

chandlerprall Feb 22, 2021

chandlerprall commented Feb 19, 2021

kibanamachine commented Feb 19, 2021

akashgp09 commented Feb 23, 2021

chandlerprall left a comment

akashgp09 commented Feb 26, 2021

cchaos commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

cchaos commented Mar 2, 2021

cchaos left a comment

kibanamachine commented Mar 2, 2021

chandlerprall left a comment

cchaos commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

akashgp09 commented Mar 2, 2021 •

edited

Loading

[EuiForm, EuiCallout] Added focus state to error callout #4497

[EuiForm, EuiCallout] Added focus state to error callout #4497

Conversation

akashgp09 commented Feb 6, 2021 • edited Loading

Summary

Screenshot

Checklist

kibanamachine commented Feb 6, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

chandlerprall Feb 8, 2021

Choose a reason for hiding this comment

chandlerprall Feb 8, 2021

Choose a reason for hiding this comment

chandlerprall Feb 8, 2021

Choose a reason for hiding this comment

chandlerprall Feb 8, 2021

Choose a reason for hiding this comment

chandlerprall commented Feb 9, 2021

cchaos commented Feb 10, 2021

akashgp09 commented Feb 10, 2021

akashgp09 commented Feb 18, 2021

Added focus to Error

Before

After

chandlerprall commented Feb 18, 2021

kibanamachine commented Feb 18, 2021

cchaos commented Feb 19, 2021 • edited Loading

akashgp09 commented Feb 19, 2021 • edited Loading

akashgp09 commented Feb 19, 2021

cchaos commented Feb 19, 2021

chandlerprall Feb 19, 2021

Choose a reason for hiding this comment

akashgp09 Feb 22, 2021

Choose a reason for hiding this comment

chandlerprall Feb 22, 2021

Choose a reason for hiding this comment

chandlerprall commented Feb 19, 2021

kibanamachine commented Feb 19, 2021

akashgp09 commented Feb 23, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

akashgp09 commented Feb 26, 2021

cchaos commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

cchaos commented Mar 2, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 2, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

cchaos commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

akashgp09 commented Mar 2, 2021 • edited Loading

akashgp09 commented Feb 6, 2021 •

edited

Loading

cchaos commented Feb 19, 2021 •

edited

Loading

akashgp09 commented Feb 19, 2021 •

edited

Loading

akashgp09 commented Mar 2, 2021 •

edited

Loading