Fix focus state regression of icons in EuiFormControlLayout. by cjcenizal · Pull Request #898 · elastic/eui

cjcenizal · 2018-06-04T13:51:29Z

Fixes #893

Thanks for catching this @cchaos!

I tested the following scenarios:

Clicking the icon in a EuiSelect now opens the select and gives it focus
Clicking the icon in a EuiFieldPassword gives focus to the input
Clicking both the clear and caret icons in EuiComboBox still has the original desired behavior of clearing/toggling the EuiComboBox

I also added a note to the CHANGELOG under 0.0.50 noting this regression.

cchaos

Tested in Chrome and IE, seems to be working.

May want to consider adding focusable="false" on the actual EuiIcon components or else you'll get tabbing like so in IE:

cjcenizal · 2018-06-04T19:17:53Z

@cchaos Ready for your 👀 again!

cchaos

I was able to follow this much better. I just had some comments/concerns mainly about classes.

cchaos · 2018-06-04T20:13:07Z

src/components/form/form_control_layout/form_control_layout_custom_icon.js

+  ...rest
+}) => {
+  const classes = classNames(
+    'euiFormControlLayoutIcons__customIcon',


I would think this className should be euiFormControlLayoutCustomIcon to keep with our conventions.

cchaos · 2018-06-04T20:20:52Z

src/components/form/form_control_layout/form_control_layout_custom_icon.js

+  return (
+    <EuiIcon
+      aria-hidden="true"
+      className={classes}


Hmmm, my worry here is that the classes are added directly to the SVG here, but are added to the button if there's a click. It seems we could have misleading/incorrect styles because they're completely different elements with the same classes. Can we maybe just change the outer element to be span or button?

cchaos · 2018-06-04T20:21:43Z

src/components/form/form_control_layout/form_control_layout_custom_icon.js

+EuiFormControlLayoutCustomIcon.propTypes = {
+  className: PropTypes.string,
+  onClick: PropTypes.func,
+  type: PropTypes.string,


iconType vs type ?

cchaos · 2018-06-04T20:22:12Z

src/components/form/form_control_layout/form_control_layout_clear_button.js

+  onClick,
+  ...rest
+}) => {
+  const classes = classNames('euiFormControlLayoutIcons__clearButton', className);


I would think this className should be euiFormControlLayoutClearButton to keep with our conventions.

cchaos · 2018-06-04T20:22:35Z

src/components/form/form_control_layout/form_control_layout_clear_button.js

+      {...rest}
+    >
+      <EuiIcon
+        className="euiFormControlLayoutIcons__clearButtonIcon"


This one would need to change too.

cchaos · 2018-06-04T20:23:47Z

src/components/form/form_control_layout/_form_control_layout_icons.scss

+  right: $euiFormControlPadding;
+}
+
+  .euiFormControlLayoutIcons__clearButton {


I would split these into their own files as we do other components.

cchaos · 2018-06-04T20:26:27Z

src/components/form/form_control_layout/form_control_layout_icons.js

+
+    return (
+      <EuiFormControlLayoutClearButton
+        aria-label="Clear selections"


Label is too specific here, maybe just "Clear" or "Clear input". "Selections" (plural) won't make sense if we use this for single selection elements.

cchaos · 2018-06-04T20:29:06Z

src/components/form/form_control_layout/form_control_layout_icons.js

+      ref: iconRef,
+      side, // eslint-disable-line no-unused-vars
+      ...iconRest
+    } = iconProps


Missing ;

cchaos · 2018-06-04T20:30:49Z

src/components/form/form_control_layout/form_control_layout_icons.js

+
+    const {
+      ref: iconRef,
+      side, // eslint-disable-line no-unused-vars


I think these two is where I get confused, but I think it's just my lack of knowledge. But are you creating these or supplying these? And if you're creating the iconRef, how does one access it?

cchaos · 2018-06-04T20:35:58Z

src/components/form/form_control_layout/_form_control_layout_icons.scss

+    width: $euiSize;
+    @include size($euiSize);
+
+    .euiIcon {


Now that it's broken down further, I think we can go ahead and give the EuiIcon a custom class for this.

cjcenizal · 2018-06-04T22:29:58Z

Thanks for the review @cchaos. I've addressed your feedback, could you take another look?

cchaos

LG, tested in IE too. Just one class omission.

cchaos · 2018-06-04T23:44:25Z

src/components/form/form_control_layout/form_control_layout_custom_icon.js

+      {...rest}
+    >
+      <EuiIcon
+        aria-hidden="true"


You'd want the same className here as well.

Maybe even just create a variable for the EuiIcon node

It looks like it adjusts the position of the icon specifically when it's inside a button, so I don't think it's needed here. Am I missing something?

Oh yeah... I guess I was thinking long-term in case we or a consumer needed to target that icon specifically, it would be nice to have a class. Meaning, they would actually be the same class .euiFormControlLayoutCustomIcon__icon that would only have specific styles (for now) within .euiFormControlLayoutCustomIcon--clickable

Oh I see. I think I'll punt on this for now since I don't think we have a compelling need for it yet, and it will be simple to add once we do.

…lastic#898)" This reverts commit b78e76d. # Conflicts: # CHANGELOG.md # src/components/form/file_picker/_file_picker.scss # src/components/form/form_control_layout/_mixins.scss

…Layout. (elastic#898)"" This reverts commit 7d645ee.

cjcenizal requested a review from cchaos June 4, 2018 13:51

cjcenizal force-pushed the bug/form-control-layout-icon-focus branch 2 times, most recently from 473dc12 to 51a8acb Compare June 4, 2018 13:54

cchaos approved these changes Jun 4, 2018

View reviewed changes

cjcenizal mentioned this pull request Jun 4, 2018

EuiIcon no longer takes focus in Edge and IE unless tabIndex is specified as a value other than "-1" #900

Merged

cjcenizal requested a review from chandlerprall June 4, 2018 19:44

cchaos suggested changes Jun 4, 2018

View reviewed changes

cchaos mentioned this pull request Jun 4, 2018

(Mostly) Form Fixes #894

Merged

2 tasks

cjcenizal force-pushed the bug/form-control-layout-icon-focus branch from 6d6e4f8 to d3a21c4 Compare June 4, 2018 22:30

cchaos approved these changes Jun 4, 2018

View reviewed changes

cjcenizal added 4 commits June 4, 2018 17:03

Fix focus state regression of icons in EuiFormControlLayout.

cc4d4e1

Refactor EuiFormControlLayout for readability and clarity.

66ca18d

Break up SCSS into multiple files and improve class names.

8449e6a

Normalize DOM structure of EuiFormControlLayoutCustomIcon.

998e419

cjcenizal force-pushed the bug/form-control-layout-icon-focus branch from d3a21c4 to 998e419 Compare June 5, 2018 00:07

cjcenizal merged commit b78e76d into elastic:master Jun 5, 2018

cjcenizal deleted the bug/form-control-layout-icon-focus branch June 5, 2018 04:37

cchaos pushed a commit to cchaos/eui that referenced this pull request Jun 5, 2018

Revert "Revert "Fix focus state regression of icons in EuiFormControl…

a35448d

…Layout. (elastic#898)"" This reverts commit 7d645ee.

cchaos mentioned this pull request Jun 5, 2018

Fixing form layout icon vertical alignment #904

Merged

Conversation

cjcenizal commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Jun 4, 2018

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Jun 4, 2018

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cjcenizal commented Jun 4, 2018 •

edited

Loading