-
Notifications
You must be signed in to change notification settings - Fork 861
[Form controls] Added alert icon indicators for invalid state
#5738
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
Conversation
which is passed through to `EuiFormControlLayoutIcon` which renders the `alert` icon in red
- EuiFieldNumber, EuiFieldPassword, EuiFieldText, EuiSelect, EuiSuperSelect - EuiFieldSearch attempts to create a new class for the number of icons
…to reduce output syles
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
- Fixed EuiSuperSelect border radius with append/prepend elastic#5442 - Fixed EuiSuperSelect not respecting `readOnly` elastic#3510
|
Ok, this PR is ready for review as I go through the final checklist. Please be sure to read through the PR summary as this PR only tackles some of the form controls. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
| <eui-validatable-control | ||
| isinvalid="true" | ||
| > | ||
| <input | ||
| class="euiFieldNumber" | ||
| class="euiFieldNumber euiFormControlLayout--1icons" | ||
| type="number" | ||
| /> |
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.
I don't know why in the snapshots it's not adding that aria-invalid to the <input>. The one place I did see it get added was in the EuiSuggest snapshot https://github.com/elastic/eui/pull/5738/files#diff-4cd42a1237d4c776e9fd1140e106d4ba0d063ec3a7010a7646ca74236b040f8a
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.
I think because EuiValidatableControl is getting mocked in the test file:
eui/src/components/form/field_number/field_number.test.tsx
Lines 22 to 24 in d723d95
| jest.mock('../validatable_control', () => ({ | |
| EuiValidatableControl: 'eui-validatable-control', | |
| })); |
So the cloneElement additions don't happen. Not sure if that mock is still necessary.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
elizabetdev
left a comment
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.
|
No. 1 should be fixed, I was re-calculating the spacing between icons to be smaller in compressed but in fact it never was changing. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
1Copenut
left a comment
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.
LGTM! The Color Palette Picker is receiving the isInvalid prop correctly, but isn't announcing itself as an invalid text field b/c of the button + listbox structure. Not a problem, just an item we should review and strategize ways to announce as invalid in another issue.
thompsongl
left a comment
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.
Just the one question. Everything is looking good!
Co-authored-by: Greg Thompson <[email protected]>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
This reverts commit fce7b90. # Conflicts: # src-docs/src/views/form_controls/display_toggles.js
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5738/ |

Partially tackles #2017
[EuiFormControlLayout]
isInvalidprop which is passed through toEuiFormControlLayoutIconwhich renders thealerticon in red (danger)isDropdownto create and control thearrowDownicon (better for quickly creating this internally and handles visibility based onreadOnlyanddisabledstatescoloras a possible option for theIconObject(more configuration options that I needed for color picker)Passed the
isInvalidthrough for only the following form controlsTo test, I've set all the Display Toggles to be on by default (I'll revert this before merging).
New Sass method of applying left/right padding based on number of icons
getFormControlClassNameForIconCountlocalized service for counting icons based on boolean state variablesclassNameoutput from rendering component-specific to generic to reduce output styles. So instead of.euiFieldText--isInvalid.euiFieldText--hasIconand all those combinations of state-produced classes, the above service passes backeuiFormControlLayout--2iconsthat's applied to the form control. So we only have 5 (1-5 number of icons possible) classes total for handling padding vs how every many permutations per control.[EuiColorPicker] Fixed usage of EuiFormControlLayout to not double
In order for the EuiColorPicker to have an icon on the left (color swatch) and the dropdown arrow on the right, it had to create a second EuiFormControlWrapper to add it. Now with the ability to just say
isDropdownto add the arrow icon, we no longer need the second wrapper. But instead of being able to just remove the manual wrapper I had to setcontrolOnlyon the EuiFieldText component because we needed to support theclearprop.[EuiSelect] & [EuiSuperSelect] Updated to new props of EuIFormControlLayout
These components now use the
isDropdownprop instead of a customicon. I was also able to fix a couple other quick issues:prependandappendneeds it's border-radius fixed in Amsterdam #5442 EuiSuperSelect border radius with append/prependreadOnlyprop #3510 EuiSuperSelect not respectingreadOnlyChecklist
[ ] Checked for breaking changes and labeled appropriately