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

Allow setting autoFocus on radio group option #1117

Merged
merged 4 commits into from
Aug 16, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 15, 2018

EuiRadioGroup in the following situation exposes the need to pass autoFocus to EuiRadio

  • Inside of EuiContextMenu (or another component that sets focus on open)
  • EuiRadioGroup is first component in a panel
  • The first option is disabled
  • The first option has a EuiIconTip in the label

The problem is that the tooltip opens when the panel is opened.

Setting autoFocus on the first enabled option solves this problem

screen shot 2018-08-15 at 8 15 31 am

@nreese nreese requested review from chandlerprall and cchaos August 15, 2018 16:02
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for adding this. I think to save others having the same issue with more props, we can just pass the rest of the props per item down since we don't need to do any extraneous manipulation to this prop.

@@ -27,6 +27,7 @@ export const EuiRadioGroup = ({
disabled={disabled || option.disabled}
onChange={onChange.bind(null, option.id, option.value)}
compressed={compressed}
autoFocus={option.autofocus}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just need to add a ...rest for options like we have started doing for most components that accept options as objects:

    const items = options.map((option, index) => {
      const {
        [props]
        ...optionRest
      } = option;

      return (
        <EuiRadio
          [props]
          { ...optionRest }
        />
      );
    });

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Agree with @cchaos's review using ...rest on the option, otherwise this LGTM

compressed={compressed}
autoFocus={autoFocus}
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the autoFocus, id, label, and value props as they are now captured by optionRest

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

changes LGTM!

@@ -71,6 +73,7 @@ EuiRadio.propTypes = {
* when `true` creates a shorter height radio row
*/
compressed: PropTypes.bool,
autoFocus: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think you still don't need these? autofocus is html prop. Unless React expects it to be passed as a camel case prop autoFocus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not work unless camel cased

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

@cchaos
Copy link
Contributor

cchaos commented Aug 15, 2018

I'm just having an issue with IE11... of course... Where it seems to work but then when you try to interact with it via keyboard, it bumps you back to the top of the page.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Let's just get this in and worry about IE later, unless you've already started looking into it.

@nreese nreese merged commit 06d6ab5 into elastic:master Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants