chore(warnings): Fix warnings in Form and Modal #626

jeffcarbs · 2016-10-05T14:10:46Z

Fixes warnings in two of the components referenced here: #599

codecov-io · 2016-10-05T14:15:47Z

Current coverage is 100% (diff: 100%)

Merging #626 into master will not change coverage

@@           master   #626   diff @@
====================================
  Files         119    119          
  Lines        1881   1884     +3   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         1881   1884     +3   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update ddb6349...1f3eebe

layershifter · 2016-10-05T15:03:21Z

Note, I added tests for Form's bool props in #584.

levithomason · 2016-10-05T16:03:54Z

src/modules/Modal/Modal.js

-    const portalProps = _.pick(rest, _.keys(Portal.propTypes))
+
+    // Remove portal-handled props from 'rest' and pass them through to Portal
+    const portalProps = _.reduce(rest, (memo, value, key) => {


Just a suggestion here. This adds another loop, but is a little clearer (at least to my eyes) and does not mutate any objects:

const unhandled = getUnhandledProps(Modal, this.props) const portalPropKeys = _.keys(Portal.propTypes) const portalProps = _.pick(unhandled, portalPropKeys) const rest = _.omit(unhandled, portalPropKeys)

levithomason · 2016-10-05T16:11:22Z

test/specs/elements/Input/Input-test.js

@@ -62,4 +63,23 @@ describe('Input', () => {
      })
    })
  })
+
+  describe('onChange', () => {
+    it('handles onChange', () => {


This should be covered by the conformance test for events. It runs every component through every event handler and asserts that it was called, and was called with the appropriate event shape as the first arg.

levithomason · 2016-10-05T16:14:57Z

src/elements/Input/Input.js

+    // React gives a warning if no onChange is given to a controlled input. We
+    // handle the onChange on the wrapping element, so we don't actually need
+    // onChange on the input itself but let's pass a noop to suppress the warning.
+    onChange: _.noop,


Perhaps we instead add onChange to the htmlInputPropNames? This way, it is applied to the correct place and suppresses the warning only when it should be suppressed (user passed value/onChange). My concern is that we're swallowing the warning. If the user adds <Input value='' />, they should see the warning but they won't with the current change here.

That makes sense, although adding it to the htmlInputPropNames actually causes it to be called twice. I think we need to do a similar thing here as the portalProps, where any prop we pass through to the input isn't included in the rest. e.g.

// Currently: const rest = getUnhandledProps(Input, props) const inputProps = _.pick(props, htmlInputPropNames) // Change to: const unhandled = getUnhandledProps(Input, props) const htmlInputProps = _.pick(unhandled, htmlInputPropNames) const rest = _.omit(unhandled, htmlInputPropNames)

I think that is true? Just make sure all the examples work/look how they should. SUI relies partially on the classNames and partially on the input props to do what it does for the Input component.

There are gonna be some exceptions for things like disabled, which adds opacity to the input. It can't be both a class and an attribute or it is doubly transparent. Also, readOnly I believe must be on the input to prevent entries, but not required as a className.

Actually, this causes the handles events transparently test to fail. I think that test tries to call the event (onChange in this case) on the top-level component (the Input in this case). Since the onChange is now actually being passed as a prop to the htmlInput, it's not being called when simulated on the Input.

Thoughts on how to handle this? I agree it would be better to not suppress the warning, so ideally we can improve the test, assuming I'm understanding the issue correctly.

Ugh, yep. That test is a little to naive. We could teach it where to put the handler (pass an argument somehow), but that would be pretty ugly I think.

I'll give this more thought and see what I can come up with.

levithomason · 2016-10-05T16:16:34Z

test/specs/modules/Modal/Modal-test.js

@@ -104,6 +104,12 @@ describe('Modal', () => {
      shallow(<Modal open={false} />)
        .find('Portal')
        .should.have.prop('open', false)
+
+      // Does not pass this prop to the portal child


Suggest pulling this into it's own it() block to keep assertions isolated.

jeffcarbs · 2016-10-05T18:34:39Z

Regarding my comment about the "handles events transparently" above, I updated the test slightly to account for this situation. I think it makes sense but let me know if not.

The idea behind the test (imho) is to ensure that if a user passes an event handler as a prop to the top-level component it is fired when that event takes place within that component. In this case, we're binding the event to a more appropriate sub-component that is the only element that will actually cause the event to be fired in an actual browser. So this test now is able to simulate a more real-world scenario.

The only negative is that within other tests if you did .find(Input).simulate('change') it wouldn't trigger the onChange for the component as one might expect because the event needs to be simulated on the html input within.

Thoughts?

levithomason · 2016-10-05T18:51:24Z

test/specs/commonTests.js

@@ -295,7 +298,7 @@ export const isConformant = (Component, options = {}) => {
        //                   ^ was not called on "blur"
        const leftPad = ' '.repeat(constructorName.length + listenerName.length + 3)

-        handlerSpy.called.should.equal(true,
+        handlerSpy.calledOnce.should.equal(true,
          `<${constructorName} ${listenerName}={${handlerName}} />\n` +
          `${leftPad} ^ was not called on "${eventName}".` +


Update required: "was not called on once"

levithomason · 2016-10-05T18:53:05Z

Test changes look good to me! I think if someone is writing tests for a component, they should be responsible for find()ing the correct component to fire the event on. Seems sane.

I added one last request to update the event assertion error to match the updated calledOnce assertion. Other than this, I think we're there!

levithomason · 2016-10-05T19:16:32Z

Looks good, will merge on pass

levithomason reviewed Oct 5, 2016

View reviewed changes

levithomason added the needs author feedback label Oct 5, 2016

jeffcarbs added 2 commits October 5, 2016 14:01

feat(Modal): fix warnings

077f90a

feat(Input): Fix warnings

d90355a

jeffcarbs force-pushed the feature/warnings branch from 272d869 to 6a1ea38 Compare October 5, 2016 18:26

jeffcarbs added ready for review and removed needs author feedback labels Oct 5, 2016

levithomason reviewed Oct 5, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Oct 5, 2016

Update based on feedback

1f3eebe

jeffcarbs force-pushed the feature/warnings branch from 6a1ea38 to 1f3eebe Compare October 5, 2016 19:12

jeffcarbs added ready for review and removed needs author feedback labels Oct 5, 2016

levithomason approved these changes Oct 5, 2016

View reviewed changes

levithomason merged commit 835dd0a into master Oct 5, 2016

levithomason deleted the feature/warnings branch October 5, 2016 19:21

layershifter mentioned this pull request Oct 6, 2016

docs(Example): Fix warnings #599

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(warnings): Fix warnings in Form and Modal #626

chore(warnings): Fix warnings in Form and Modal #626

jeffcarbs commented Oct 5, 2016

codecov-io commented Oct 5, 2016 •

edited

Loading

layershifter commented Oct 5, 2016

levithomason Oct 5, 2016 •

edited

Loading

levithomason Oct 5, 2016

levithomason Oct 5, 2016

jeffcarbs Oct 5, 2016 •

edited

Loading

levithomason Oct 5, 2016

jeffcarbs Oct 5, 2016

levithomason Oct 5, 2016

levithomason Oct 5, 2016

jeffcarbs commented Oct 5, 2016

levithomason Oct 5, 2016

jeffcarbs Oct 5, 2016

levithomason commented Oct 5, 2016

levithomason commented Oct 5, 2016

chore(warnings): Fix warnings in Form and Modal #626

chore(warnings): Fix warnings in Form and Modal #626

Conversation

jeffcarbs commented Oct 5, 2016

codecov-io commented Oct 5, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

layershifter commented Oct 5, 2016

levithomason Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs Oct 5, 2016 • edited Loading

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

jeffcarbs commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Oct 5, 2016

levithomason commented Oct 5, 2016

codecov-io commented Oct 5, 2016 •

edited

Loading

levithomason Oct 5, 2016 •

edited

Loading

jeffcarbs Oct 5, 2016 •

edited

Loading