[SelectInput] Add tabIndex prop #10345

aigoncharov · 2018-02-19T11:40:57Z

aigoncharov · 2018-02-19T13:53:47Z

@oliviertassinari Do you want me to add any tests? I thought about having a test about passing a tabIndex and then checking that it's there indeed, but it kind of seems like something pretty trivial and therefore unnecessary.

oliviertassinari · 2018-02-19T14:15:57Z

it kind of seems like something pretty trivial and therefore unnecessary

And yet, it's not working with the latest release :).

aigoncharov · 2018-02-19T14:23:31Z

@oliviertassinari it's not? I see only code coverage tests failing

mbrookes

Thanks for your PR! Some suggested changes for consistency with other components. Please could you also add a test for this behaviour?

mbrookes · 2018-02-19T14:54:06Z

src/Select/SelectInput.js

@@ -413,6 +419,10 @@ SelectInput.propTypes = {
   * You can only use it when the `native` property is `false` (default).
   */
  renderValue: PropTypes.func,
+  /**
+   * Tab index of the component
+   */


This prop can probably be @ignoreed, since it's a standard DOM attribute (albeit being applied to a nested div), but also since this is an internal component.

mbrookes · 2018-02-19T14:54:08Z

src/Select/SelectInput.js

+  /**
+   * Tab index of the component
+   */
+  tabIndex: PropTypes.number,


It should also accept a string. You can follow the Chip example:

tabIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

mbrookes · 2018-02-19T14:54:38Z

src/Select/SelectInput.js

@@ -154,6 +154,7 @@ class SelectInput extends React.Component {
      open: openProp,
      readOnly,
      renderValue,
+      tabIndex,


tabIndex: tabIndexProp,

mbrookes · 2018-02-19T14:58:40Z

src/Select/SelectInput.js

@@ -260,6 +261,11 @@ class SelectInput extends React.Component {

    const MenuMinWidth = this.displayNode && !autoWidth ? this.displayNode.clientWidth : undefined;

+    let tabIndexFinal = tabIndex || 0;


let tabIndex = tabIndexProp || 0;

mbrookes · 2018-02-19T14:59:29Z

src/Select/SelectInput.js

+    if (disabled) {
+      tabIndexFinal = null;
+    }
+


- if (disabled) { - tabIndexFinal = null; - }

mbrookes · 2018-02-19T15:00:44Z

src/Select/SelectInput.js

@@ -276,7 +282,7 @@ class SelectInput extends React.Component {
          }}
          data-mui-test="SelectDisplay"
          aria-pressed={open ? 'true' : 'false'}
-          tabIndex={disabled ? null : 0}
+          tabIndex={tabIndexFinal}


tabIndex={disabled ? null : tabIndex}

aigoncharov · 2018-02-20T11:01:09Z

@mbrookes sure. I'll make these changes, though it's gonna take couple days to find a time slot for this. Thanks for the review!

ok

Per TypeScript definition and React usage: - https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html#why-are-we-changing-this - https://github.com/facebook/react/blob/86914cb30a7654d13a17d34a3b3a5af97c2d2855/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js#L439

oliviertassinari changed the title ~~[Closes #10344] [Select] tabIndex is hardcoded and can not be changed~~ [Select] tabIndex is hardcoded and can not be changed Feb 19, 2018

oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Feb 19, 2018

mbrookes previously requested changes Feb 19, 2018

View reviewed changes

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 19, 2018

mbrookes changed the title ~~[Select] tabIndex is hardcoded and can not be changed~~ [SelectInput] Add tabIndex prop Feb 19, 2018

oliviertassinari self-assigned this Feb 23, 2018

oliviertassinari added new feature New feature or request and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 23, 2018

oliviertassinari force-pushed the v1-beta branch from 5863733 to e5e35a8 Compare February 23, 2018 22:22

oliviertassinari force-pushed the v1-beta branch from e5e35a8 to 58c4a61 Compare February 23, 2018 22:33

keenondrums and others added 4 commits February 23, 2018 23:47

[Closes mui#10344] [Select] tabIndex is hardcoded and can not be changed

65e5c36

Code style fixes

1c9c963

clean up

ce8e91c

oliviertassinari force-pushed the v1-beta branch from 58c4a61 to df3fe0f Compare February 23, 2018 22:49

oliviertassinari merged commit 592e6be into mui:v1-beta Feb 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SelectInput] Add tabIndex prop #10345

[SelectInput] Add tabIndex prop #10345

aigoncharov commented Feb 19, 2018 •

edited by oliviertassinari

aigoncharov commented Feb 19, 2018

oliviertassinari commented Feb 19, 2018

aigoncharov commented Feb 19, 2018

mbrookes left a comment

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018

mbrookes Feb 19, 2018 •

edited

aigoncharov commented Feb 20, 2018

		@@ -260,6 +261,11 @@ class SelectInput extends React.Component {

		const MenuMinWidth = this.displayNode && !autoWidth ? this.displayNode.clientWidth : undefined;

		let tabIndexFinal = tabIndex \|\| 0;

[SelectInput] Add tabIndex prop #10345

[SelectInput] Add tabIndex prop #10345

Conversation

aigoncharov commented Feb 19, 2018 • edited by oliviertassinari

aigoncharov commented Feb 19, 2018

oliviertassinari commented Feb 19, 2018

aigoncharov commented Feb 19, 2018

mbrookes left a comment

Choose a reason for hiding this comment

mbrookes Feb 19, 2018 • edited

Choose a reason for hiding this comment

mbrookes Feb 19, 2018 • edited

Choose a reason for hiding this comment

mbrookes Feb 19, 2018 • edited

Choose a reason for hiding this comment

mbrookes Feb 19, 2018 • edited

Choose a reason for hiding this comment

mbrookes Feb 19, 2018

Choose a reason for hiding this comment

mbrookes Feb 19, 2018 • edited

Choose a reason for hiding this comment

aigoncharov commented Feb 20, 2018

aigoncharov commented Feb 19, 2018 •

edited by oliviertassinari

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited

mbrookes Feb 19, 2018 •

edited