Skip to content

Conversation

@thebuilder
Copy link

Fixes warning in 15.2.0

Fixes warning in 15.2.0
@thebuilder thebuilder mentioned this pull request Jul 5, 2016
@thebuilder
Copy link
Author

I did this change in the Github editor, and haven't tested it. :)

@hartzis
Copy link
Collaborator

hartzis commented Jul 6, 2016

I had a feeling react was going to do something about this. It is a little weird to just spread all the props. I don't know what the best solution is here, but I'd like to get this running on the example to test it at least.

@thebuilder
Copy link
Author

I agree, but it seems like performing the spread is the simplest way.
The alternative is to clone the object, and delete the internal keys:

const divProps = Object.assign({}, props);
delete divProps.layout;

@hartzis
Copy link
Collaborator

hartzis commented Jul 6, 2016

The clone and delete are very explicit, I like the use here
https://github.com/JedWatson/react-tappable/blob/master/src/getComponent.js#L45

Long term I still would like to attach swipeables props to the only child
via cloneElement or something similar like that. To avoid this whole issue
hopefully.

@emilyaviva
Copy link

I can confirm that this is an issue. I'm doing something like this:

class MyComponent extends React.Component {
  render () {
    const {
      onSwipedRight
    } = this.props
    return (
      <Swipeable nodeName='article' onSwipedRight={onSwipedRight}>
        <div className='headline'>blah blah blah</div>
        <p>this is another child element</p>
      </Swipeable>
    )
  }
}
MyComponent.propTypes = {
  onSwipedRight: React.PropTypes.func
}

and I'm getting this error message back:

Unknown props `nodeName`, `onSwipedRight`, `flickThreshold`, `delta`, `preventDefaultTouchmoveEvent` on <article>

I'm not really thrilled with the idea of putting all the props in to be explicitly deleted. That doesn't seem very React-like, somehow.

@hartzis
Copy link
Collaborator

hartzis commented Jul 7, 2016

@emilyaviva I agree I don't really 'like' the delete either, just found an example of it in another heavily used touch/swipe library. I do have a long term solution, with cloneElement, but it will require an api and major version change.

With the delete method we also avoid name spacing/assigning a bunch of variables that don't actually get used. I think it may be the better short term solution currently.

@thebuilder thank you for this PR and catching the issue, but do you think you could change/update this PR to do the clone/delete method? Let me know if you are unable and/or don't have the time, I could probably get around to it this weekend. Cheers.

@hartzis
Copy link
Collaborator

hartzis commented Jul 12, 2016

solved with #44

@hartzis hartzis closed this Jul 12, 2016
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