[SelectField] Fix TouchRipple effect when SelectField is disabled #6286

solkaz · 2017-03-05T16:57:18Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

…uchRipple prop

oliviertassinari

Looks like we were misleading. The true issue seems to have nothing to do with disableTouchRipple. I would avoid doing any changes on the property. But address the disabled forward to begin with.

oliviertassinari · 2017-03-05T17:11:39Z

src/DropDownMenu/DropDownMenu.js

@@ -107,6 +107,10 @@ class DropDownMenu extends Component {
     */
    className: PropTypes.string,
    /**
+     * If true, the icon element's ripple effect will be disabled


Missing dot at the end.

oliviertassinari · 2017-03-05T17:12:57Z

src/DropDownMenu/DropDownMenu.spec.js

+      );
+      assert.strictEqual(wrapper.find(TouchRipple).length, 0, 'should not contain a TouchRipple');
+    });
+    it('has a touchRipple effect if disableTouchRipple is undefined and disabled={false}', () => {


We separate the tests with a blank line.

oliviertassinari · 2017-03-05T17:14:36Z

src/IconButton/IconButton.spec.js

+      );
+      assert.strictEqual(wrapper.find(TouchRipple).length, 1, 'should contain a TouchRipple descendent');
+    });
+    it('disables the ripple effect when disableTouchRipple={true}', () => {


I think that it would be better to use shorter description. We can always read the tests to have more details. What about disables the ripple effect here. The comment applies to the other description.

oliviertassinari · 2017-03-05T17:15:13Z

src/SelectField/SelectField.spec.js

+import {mount} from 'enzyme';
+import {assert} from 'chai';
+import getMuiTheme from '../styles/getMuiTheme';
+


We avoid using blank line in the import list.

oliviertassinari · 2017-03-05T17:15:28Z

src/SelectField/SelectField.js

@@ -36,6 +36,10 @@ class SelectField extends Component {
     */
    children: PropTypes.node,
    /**
+     * If true, the icon element's ripple effect will be disabled


Missing dot at the end of the description.

oliviertassinari · 2017-03-05T17:19:47Z

src/DropDownMenu/DropDownMenu.spec.js

@@ -31,6 +32,43 @@ describe('<DropDownMenu />', () => {
    });
  });

+  describe('prop: disabled', () => {
+    it('disables the touchRipple effect if disableTouchRipple is undefined and disabled={true}', () => {
+      const wrapper = mountWithContext(


I don't think that we need to use the mount API. The shallow API should be enough for all the new tests.

The reason I used mount is due to my testing strategy, as I had checked for the existence of a TouchRipple component. There definitely has to be a better strategy though

Yes, it's really around what you are trying to test. That's fair. We could have been testing that the right property is set, but I guess that an integration test is better suited here 👍 .

Would you say that the unit tests for SelectField are extraneous? Because the behavior is dependent on DropDownMenu

oliviertassinari · 2017-03-05T17:20:19Z

src/DropDownMenu/DropDownMenu.js

@@ -364,6 +370,7 @@ class DropDownMenu extends Component {
            {displayValue}
          </div>
          <IconButton
+            disableTouchRipple={disableTouchRipple === undefined ? disabled : disableTouchRipple}


That logic can't be working.

The passing tests are suggesting that the issue is coming from somewhere else. And true, from what I can see the true issue is that we don't forward the disabled property to the IconButton.
That piece of logic is already inside the EnhancedButton component. No need for duplication.

So then, what I need to is to pass <DropDownMenu />'s disabled prop to <IconButton />, which would then disable the ripple effect?

Yes indeed. Can you try that?

oliviertassinari · 2017-03-05T17:23:28Z

src/DropDownMenu/DropDownMenu.js

@@ -364,6 +370,7 @@ class DropDownMenu extends Component {
            {displayValue}
          </div>
          <IconButton
+            disableTouchRipple={disableTouchRipple === undefined ? disabled : disableTouchRipple}
            tabIndex={this.props.disabled ? -1 : undefined}


I don't think that we need that once we forward the disabled property as it's already inside the EnhancedButton component.

oliviertassinari · 2017-03-05T17:27:43Z

Ping @elazzabi

…to IconButton

oliviertassinari

Aside from my comments, it's good to be merged. Good work so far 👍 .

oliviertassinari · 2017-03-06T22:09:30Z

src/DropDownMenu/DropDownMenu.js

@@ -176,6 +176,7 @@ class DropDownMenu extends Component {
    animated: true,
    autoWidth: true,
    disabled: false,
+    disableKeyboardFocus: false,


What is this property for?

I'm not sure, I think that snuck in there. Definitely shouldn't be in there

oliviertassinari · 2017-03-06T22:10:24Z

src/DropDownMenu/DropDownMenu.js

@@ -364,7 +365,8 @@ class DropDownMenu extends Component {
            {displayValue}
          </div>
          <IconButton
-            tabIndex={this.props.disabled ? -1 : undefined}
+            disabled={disabled}
+            tabIndex={disabled ? -1 : undefined}


The EnhancedButton already has a logic to implement that. I think that you can remove this line.

tabIndex: disabled || disableKeyboardFocus ? -1 : tabIndex,

oliviertassinari · 2017-03-06T22:10:53Z

src/DropDownMenu/DropDownMenu.spec.js

@@ -31,6 +31,15 @@ describe('<DropDownMenu />', () => {
    });
  });

+  describe('prop: disabled', () => {
+    it('disables the ripple effect', () => {


it('should forward the property', () => {

oliviertassinari · 2017-03-06T22:11:30Z

src/IconButton/IconButton.spec.js

@@ -72,4 +77,12 @@ describe('<IconButton />', () => {
      assert.include(wrapper.props().style, hoveredStyle);
    });
  });
+  describe('prop: disabled', () => {
+    it('disables the ripple effect', () => {


it('should disable the ripple effect', () => {

oliviertassinari · 2017-03-06T22:12:20Z

src/IconButton/IconButton.spec.js

+  describe('prop: disabled', () => {
+    it('disables the ripple effect', () => {
+      const wrapper = mountWithContext(
+        <IconButton disabled={true} />


Could we add another test with disabled={false} to makes sure (wrapper.find(TouchRipple).length === 1?

oliviertassinari · 2017-03-06T22:13:19Z

src/SelectField/SelectField.spec.js

+import SelectField from './SelectField';
+import TouchRipple from '../internal/TouchRipple';
+
+describe('<SelectField />', () => {


Sweet, it's increasing the coverage 👍 .

oliviertassinari · 2017-03-06T23:07:43Z

@Shahrukh-Zindani Use the squash and merge option next time. 16 commits in the git history is quite a mess.

oliviertassinari · 2017-03-06T23:08:26Z

@solkaz Thanks 👍.

Shahrukh-Zindani · 2017-03-07T03:40:05Z

Okay will be do that from now.

oliviertassinari · 2017-03-07T08:52:18Z

@Shahrukh-Zindani Thanks, that's important from my point of view.

mbrookes · 2017-03-07T12:46:32Z

@Shahrukh-Zindani Also, please don't merge something just because it has been accepted - in that case @oliviertassinari could have merged it himself. Instead, it gives an opportunity for final review.

solkaz added 10 commits March 3, 2017 21:55

[DropDownMenu] Add disableIconTouchRipple prop

e16d69b

[SelectField] Add disableIconTouchRipple prop

e4f91eb

[DropDownMenu] Added default prop value for disableIconTouchRipple

b892fbd

[SelectField] Added default prop value for disableIconTouchRipple

19fc038

[IconButton] Add tests for disableTouchRipple prop

3f45561

[DropDownMenu] Disables ripple effect based on disabled and disableTo…

eef6d3b

…uchRipple prop

[DropDownMenu] Add tests for disabled and disableTouchRipple props

9b712d9

[SelectField] Renamed disableIconTouchRipple prop to disableTouchRipple

f3d1825

[SelectField] Add tests for disabled and disableTouchRipple props

c2c26fd

Fix linter errors

05790f0

muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 5, 2017

[SelectField] Remove default for disableTouchRipple prop

5a510b3

muibot added the PR: Needs Review label Mar 5, 2017

[DropDownMenu] Explicitly disable the DropDownMenu in a test

fddb788

oliviertassinari requested changes Mar 5, 2017

View reviewed changes

oliviertassinari mentioned this pull request Mar 5, 2017

[SelectField] Fix TouchRipple effect when SelectField is disabled #6280

Closed

3 tasks

oliviertassinari removed the PR: Needs Review label Mar 5, 2017

oliviertassinari changed the title ~~Fix ripple effect when SelectField is disabled~~ [SelectField] Fix TouchRipple effect when SelectField is disabled Mar 5, 2017

oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Mar 5, 2017

solkaz added 3 commits March 6, 2017 08:33

[SelectField] Remove disableTouchRipple prop

df5dfe1

[DropDownMenu] Remove disableTouchRipple prop and pass disabled prop …

1c5692a

…to IconButton

[IconButton] Updated test for disabled prop

dd472f7

muibot added the PR: Needs Review label Mar 6, 2017

oliviertassinari requested changes Mar 6, 2017

View reviewed changes

oliviertassinari added bug 🐛 Something doesn't work and removed PR: Needs Review labels Mar 6, 2017

Addressed PR comments

a4482c4

muibot added the PR: Needs Review label Mar 6, 2017

oliviertassinari approved these changes Mar 6, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 6, 2017

solkaz mentioned this pull request Mar 6, 2017

Disable the animation on language selector until we actually have another language to select maryvilledev/codesplainUI#103

Closed

Shahrukh-Zindani merged commit e52628b into mui:master Mar 6, 2017

solkaz deleted the select-field-disable-icon-ripple branch March 6, 2017 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SelectField] Fix TouchRipple effect when SelectField is disabled #6286

[SelectField] Fix TouchRipple effect when SelectField is disabled #6286

solkaz commented Mar 5, 2017 •

edited by oliviertassinari

oliviertassinari left a comment

oliviertassinari Mar 5, 2017

oliviertassinari Mar 5, 2017

oliviertassinari Mar 5, 2017

oliviertassinari Mar 5, 2017

oliviertassinari Mar 5, 2017

oliviertassinari Mar 5, 2017

solkaz Mar 5, 2017

oliviertassinari Mar 5, 2017

solkaz Mar 6, 2017 •

edited

oliviertassinari Mar 5, 2017

oliviertassinari Mar 5, 2017

solkaz Mar 5, 2017

oliviertassinari Mar 5, 2017

solkaz Mar 5, 2017

oliviertassinari Mar 5, 2017

oliviertassinari commented Mar 5, 2017 •

edited

oliviertassinari left a comment

oliviertassinari Mar 6, 2017

solkaz Mar 6, 2017

oliviertassinari Mar 6, 2017

oliviertassinari Mar 6, 2017

oliviertassinari Mar 6, 2017

oliviertassinari Mar 6, 2017

oliviertassinari Mar 6, 2017

oliviertassinari commented Mar 6, 2017

oliviertassinari commented Mar 6, 2017

Shahrukh-Zindani commented Mar 7, 2017

oliviertassinari commented Mar 7, 2017

mbrookes commented Mar 7, 2017

[SelectField] Fix TouchRipple effect when SelectField is disabled #6286

[SelectField] Fix TouchRipple effect when SelectField is disabled #6286

Conversation

solkaz commented Mar 5, 2017 • edited by oliviertassinari

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solkaz Mar 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Mar 5, 2017 • edited

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Mar 6, 2017

oliviertassinari commented Mar 6, 2017

Shahrukh-Zindani commented Mar 7, 2017

oliviertassinari commented Mar 7, 2017

mbrookes commented Mar 7, 2017

solkaz commented Mar 5, 2017 •

edited by oliviertassinari

solkaz Mar 6, 2017 •

edited

oliviertassinari commented Mar 5, 2017 •

edited