Skip to content
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

[WIP] Add new isRequired prop that only marks invalid fields after first blur event #5244

Closed
wants to merge 8 commits into from

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 5, 2021

Summary

This is a draft PR to get CI kicking and so I have an external URL to let people test with.

Please use this discussion for this PR: #5246

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

- [ ] Checked Code Sandbox works for any docs examples

- [ ] Checked for breaking changes and labeled appropriately

  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

- Passes required attribute (+ aria-required for accessibility) to child input/control only after first blur event (prevents required empty fields from showing as invalid immediately on page load)
- work with EuiValidatableControl OOTB, no changes needed
when isRequired is set and after the first blur event is passed back from the child input and the input is empty:

- color label as invalid
- add automatic error text
+ disable isRequired switch for components that don't support it
@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

cee-chen commented Oct 6, 2021

jenkins test this

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Comment on lines +89 to +108
export const useIsRequired = ({
controlEl,
isRequired,
}: {
controlEl: HTMLConstraintValidityElement | null;
isRequired?: boolean;
}) => {
const [hasBlurred, setHasBlurred] = useState(false);
const listenForBlur = useCallback(() => setHasBlurred(true), []);

useEffect(() => {
if (isRequired && controlEl && !hasBlurred) {
controlEl.addEventListener('blur', listenForBlur, { once: true });
}
}, [isRequired, controlEl, hasBlurred, listenForBlur]);

return isRequired != null
? { required: isRequired && hasBlurred, 'aria-required': true }
: {};
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be more generic if it's useIsTouched instead and only returns a boolean to indicate if the input has been touched (blurred once). required and 'aria-required' props can then be determined in render cycles using useIsTouched.

This will also favor to expose a touched state of the input e.g. (and ideally on the parent form component too e.g.). Assuming we may need to implement that.

Comment on lines +251 to +263
optionalErrors = [
<EuiFormErrorText
key="error-required"
id={`${id}-error-required`}
className="euiFormRow__text"
>
<EuiI18n
token="euiFormRow.requiredMessage"
default="This field is required."
/>
</EuiFormErrorText>,
...(optionalErrors || []),
];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if the required error message is configurable, considering the consumer may want to show a custom error message, or no message at all but does want the invalid state.

Tip: State of all implicit errors could be exposed e.g

errors = {
    required: true,
    range: false
    ...
}

@awahab07
Copy link

This would be a great addition.

I do wish that we have the following states exposed on the form ref:

redux-form Angular Forms
touched doc doc
dirty doc doc
pristine doc doc

These are quite essential life cycle states of an average form and consumer need these to show/hide validation errors or to enable/disable form action buttons.

Among these, touched is more important because without this, consumers have to inject a lot of code to achieve the behavior. Since this PR is already hooking into onBlur of input controls, exposing this wouldn't require much work.

@kibanamachine
Copy link

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

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

cee-chen commented Apr 6, 2022

Still in my backlog to do 🙈

@cee-chen cee-chen removed the stale-pr label Apr 6, 2022
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@github-actions
Copy link

❌ We're automatically closing this PR due to lack of activity. Please comment if you feel this was done in error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants