[ListItem] Documents containerElement, changes ref to use callback #6204

lourd · 2017-02-21T10:09:43Z

Makes updates to make it clear how to wrap a ListItem with a link.
Using a string for refs is deprecated and was giving me a warning
about putting a ref on a functional component in some cases.

- Makes updates to make it clear how to wrap a ListItem with a link. - Using a string for refs is deprecated and was giving me a warning about putting a ref on a functional component in some cases.

oliviertassinari

We miss a unit test. Otherwise, it looks good 👍 .

oliviertassinari · 2017-02-21T16:37:05Z

src/List/ListItem.js

              {...other}
+              containerElement={containerElement}


I would move containerElement={containerElement} before the {...other} just for explicitness. Users should be able to overrides everything.

It's overridden by simply giving a containerElement prop. It's pulled out from the other props because it was being passed on to the label or disabled element if it wasn't pulled out. There's no logic where anything in other would be overriding containerElement.

@lourd That's going to work the same, yes. My point was just around code readability.

lourd · 2017-02-21T16:42:12Z

Thanks for reviewing so quickly! Do you mean to say a unit test is failing because of this, or it's missing coverage? If you can point out where to add it and look at other tests for reference I'll add it.

oliviertassinari · 2017-02-21T16:51:32Z

Do you mean to say a unit test is failing because of this, or it's missing coverage?

It's missing coverage. We don't have much on the master branch. You can add it inside the ListItem.test.js file.

mbrookes · 2017-02-21T18:03:06Z

src/List/ListItem.js

@@ -154,6 +154,17 @@ class ListItem extends Component {
     */
    children: PropTypes.node,
    /**
+     * The element to use as the container for the ListItem inside of. Either a


"inside of." ?

😅 it was late

lourd · 2017-02-21T18:57:20Z

Added a couple tests and switched those lines.

oliviertassinari · 2017-02-21T19:30:36Z

src/List/ListItem.spec.js

+      assert.ok(button.is('a'), 'should match an a element');
+    });
+
+    it('Should use the given ReactElement containerElement', () => {


Small typo should.

oliviertassinari · 2017-02-21T19:33:11Z

src/List/ListItem.spec.js

+        />
+      );
+      const button = wrapper.find(EnhancedButton).dive({context: {muiTheme}});
+      assert.ok(button.is('a'), 'should match an a element');


is returns a boolean, using strictEqual would be better than a truthy.

oliviertassinari · 2017-02-21T19:34:31Z

src/List/ListItem.spec.js

@@ -201,4 +201,29 @@ describe('<ListItem />', () => {
      assert.strictEqual(wrapper.state().hovered, false, 'should reset the state');
    });
  });
+
+  describe('containerElement', () => {


We have a convention where we prefix the name with prop: to describe the property expectations.

oliviertassinari · 2017-02-21T20:19:21Z

src/List/ListItem.spec.js

+        />
+      );
+      const button = wrapper.find(EnhancedButton).dive({context: {muiTheme}});
+      assert.strictEqual(button.is('a'), 'should match an a element');


assert.strictEqual(button.is('a'), true, 'should match an a element');

oliviertassinari · 2017-02-21T20:19:34Z

src/List/ListItem.spec.js

+        />
+      );
+      const button = wrapper.find(EnhancedButton).dive({context: {muiTheme}});
+      assert.strictEqual(button.is(CustomElement), 'should match the custom element');


assert.strictEqual(button.is(CustomElement), true, 'should match the custom element');

oliviertassinari · 2017-02-22T08:07:37Z

Thanks for documenting that property. It's such a useful one.

- react-intl left out to use jeremy's fork. - react-router has a major version change, requires a separate commit. - material-ui has minor changes which requires snapshot changes - see mui/material-ui#6135 mui/material-ui#6204

[ListItem] Documents containerElement, changes ref to use callback

43c002f

- Makes updates to make it clear how to wrap a ListItem with a link. - Using a string for refs is deprecated and was giving me a warning about putting a ref on a functional component in some cases.

muibot added the PR: Needs Review label Feb 21, 2017

oliviertassinari requested changes Feb 21, 2017

View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI component: list This is the name of the generic UI component, not the React module! and removed PR: Needs Review labels Feb 21, 2017

mbrookes reviewed Feb 21, 2017

View reviewed changes

muibot added the PR: Needs Review label Feb 21, 2017

mbrookes mentioned this pull request Feb 21, 2017

[docs] Add documentation for containerElement in RaisedButon #6199

Merged

oliviertassinari approved these changes Feb 21, 2017

View reviewed changes

oliviertassinari reviewed Feb 21, 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 Feb 21, 2017

muibot added the PR: Needs Review label Feb 21, 2017

oliviertassinari removed the PR: Needs Review label Feb 21, 2017

muibot 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 21, 2017

oliviertassinari reviewed Feb 21, 2017

View reviewed changes

Moves default to defaultProps, improves docs for union prop types

3d9528c

muibot added the PR: Needs Review label Feb 21, 2017

oliviertassinari 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 Feb 22, 2017

oliviertassinari merged commit 1fbb7a0 into mui:master Feb 22, 2017

lourd deleted the list-item-link branch February 23, 2017 21:01

oliviertassinari mentioned this pull request Feb 25, 2017

[docs] Normalize the container property #6238

Merged

3 tasks

weiqingtoh mentioned this pull request Mar 30, 2017

Upgrade client packages using yarn upgrade-interactive Coursemology/coursemology2#2200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ListItem] Documents containerElement, changes ref to use callback #6204

[ListItem] Documents containerElement, changes ref to use callback #6204

lourd commented Feb 21, 2017

oliviertassinari left a comment

oliviertassinari Feb 21, 2017

lourd Feb 21, 2017

oliviertassinari Feb 21, 2017

lourd commented Feb 21, 2017

oliviertassinari commented Feb 21, 2017

mbrookes Feb 21, 2017

lourd Feb 21, 2017

lourd commented Feb 21, 2017

oliviertassinari Feb 21, 2017

oliviertassinari Feb 21, 2017

oliviertassinari Feb 21, 2017

oliviertassinari Feb 21, 2017

oliviertassinari Feb 21, 2017

oliviertassinari commented Feb 22, 2017

[ListItem] Documents containerElement, changes ref to use callback #6204

[ListItem] Documents containerElement, changes ref to use callback #6204

Conversation

lourd commented Feb 21, 2017

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

lourd commented Feb 21, 2017

oliviertassinari commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd commented Feb 21, 2017

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 Feb 22, 2017