Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react-router-native/Link improvements #4816

Merged
merged 4 commits into from
Jul 5, 2017

Conversation

benstepp
Copy link
Contributor

@benstepp benstepp commented Mar 22, 2017

This change adds the following behavior to the react-router-native/Link component:

  • onPress handler can now be passed to Link
  • onPress event can now be cancelled using preventDefault()

An example illustrating the new behavior

import React, { Component } from 'react'
import { Link } from 'react-router-native'

class Example extends Component {
  onPress(event) { // this function is now called
    event.preventDefault() // this stops react-router from navigating
  }

  render() {
    return <Link to='/' onPress={this.onPress.bind(this)} />
  }
}

Fixes #4811


Notes: The react-native init included tests are failing. There is not an eslint config in react-router-native, so I tried to match the style of the other packages.

@@ -26,6 +27,8 @@
"babel-preset-react-native": "1.9.1",
"jest": "18.1.0",
"react": "15.4.1",
"react-addons-test-utils": "^15.4.2",
"react-dom": "^15.4.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need react-dom. If there are peer dep errors, you can ignore them.

Copy link
Contributor Author

@benstepp benstepp Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-dom is required for react-addons-test-utils. I opted for this because it provides an easy way to shallowly render components that require context. Shallow renders provide direct access to the component making them easier to unit test. Enzyme is also a good option that provides this.

Here's the output compared to that of react-test-renderer. I tried to find the rendered TouchableHighlight using react-test-renderer, but I couldn't seem to locate it to unit test it. I felt like the last option provided the least amount of noise while still managing to hit all of the important assertions. I'm definitely open to suggestions though.


Using react-test-renderer

import renderer from 'react-test-renderer'
const output = renderer.create(<NativeRouter><Link to='/'><View /></Link></NativeRouter>)
output

// nothing useful
{
  type: 'View',
  props: {
    accessible: true,
    accessibilityLabel: undefined,
    accessibilityComponentType: undefined,
    accessibilityTraits: undefined,
    style: [
      [Object], undefined
    ],
    onLayout: undefined,
    hitSlop: undefined,
    isTVSelectable: true,
    tvParallaxProperties: undefined,
    hasTVPreferredFocus: undefined,
    onStartShouldSetResponder: {
      [Function: bound touchableHandleStartShouldSetResponder]
      __reactBoundContext: [Object],
      __reactBoundMethod: [Function: touchableHandleStartShouldSetResponder],
      __reactBoundArguments: null,
      bind: [Function]
    },
    onResponderTerminationRequest: {
      [Function: bound touchableHandleResponderTerminationRequest]
      __reactBoundContext: [Object],
      __reactBoundMethod: [Function: touchableHandleResponderTerminationRequest],
      __reactBoundArguments: null,
      bind: [Function]
    },
    onResponderGrant: {
      [Function: bound touchableHandleResponderGrant]
      __reactBoundContext: [Object],
      __reactBoundMethod: [Function: touchableHandleResponderGrant],
      __reactBoundArguments: null,
      bind: [Function]
    },
    onResponderMove: {
      [Function: bound touchableHandleResponderMove]
      __reactBoundContext: [Object],
      __reactBoundMethod: [Function: touchableHandleResponderMove],
      __reactBoundArguments: null,
      bind: [Function]
    },
    onResponderRelease: {
      [Function: bound touchableHandleResponderRelease]
      __reactBoundContext: [Object],
      __reactBoundMethod: [Function: touchableHandleResponderRelease],
      __reactBoundArguments: null,
      bind: [Function]
    },
    onResponderTerminate: {
      [Function: bound touchableHandleResponderTerminate]
      __reactBoundContext: [Object],
      __reactBoundMethod: [Function: touchableHandleResponderTerminate],
      __reactBoundArguments: null,
      bind: [Function]
    },
    testID: undefined
  },
  children: [{
    type: 'Text',
    props: [Object],
    children: null
  }]
}


Digging into react-test-renderer

import renderer from 'react-test-renderer'
const wrapper = renderer.create(<NativeRouter><Link to='/'><View /></Link></NativeRouter>)
const ouput = wrapper._component._currentElement.props.child.props.children
output

// doesn't actually render the TouchableHighlight
{
  '$$typeof': Symbol(react.element),
  type: {
    [Function: Link]
    contextTypes: {
      router: [Function: bound checkType]
    },
    propTypes: {
      onPress: [Object],
      component: [Object],
      replace: [Object],
      to: [Object]
    },
    defaultProps: {
      component: [Object],
      replace: false
    }
  },
  key: null,
  ref: null,
  props: {
    to: '/',
    children: {
      '$$typeof': Symbol(react.element),
      type: [Function: Component],
      key: null,
      ref: null,
      props: [Object],
      _owner: null,
      _store: {}
    },
    component: {
      [Function]
      displayName: 'TouchableHighlight',
      propTypes: [Object],
      getDefaultProps: [Object],
      defaultProps: [Object]
    },
    replace: false
  },
  _owner: null,
  _store: {}
}


Using react-addons-test-utils

import ReactTestUtils from 'react-addons-test-utils'
const renderer = ReactTestUtils.createRenderer()
renderer.render(<Link to='/' />, context)
const output = renderer.getRenderOutput()
output

// easy to access with context
{
  '$$typeof': Symbol(react.element),
  type: {
    [Function]
    displayName: 'TouchableHighlight',
    propTypes: {
      accessible: [Object],
      accessibilityComponentType: [Object],
      accessibilityTraits: [Object],
      disabled: [Object],
      onPress: [Object],
      onPressIn: [Object],
      onPressOut: [Object],
      onLayout: [Object],
      onLongPress: [Object],
      delayPressIn: [Object],
      delayPressOut: [Object],
      delayLongPress: [Object],
      pressRetentionOffset: [Object],
      hitSlop: [Object],
      activeOpacity: [Object],
      underlayColor: [Object],
      style: [Function],
      onShowUnderlay: [Object],
      onHideUnderlay: [Object],
      hasTVPreferredFocus: [Object],
      tvParallaxProperties: [Object]
    },
    getDefaultProps: {
      [Function: getDefaultProps] isReactClassApproved: {}
    },
    defaultProps: {
      activeOpacity: 0.85,
      underlayColor: 'black'
    }
  },
  key: null,
  ref: null,
  props: {
    to: '/',
    replace: false,
    onPress: [Function],
    activeOpacity: 0.85,
    underlayColor: 'black'
  },
  _owner: null,
  _store: {}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more things I've found.

The shallow renderer from react-addons-test-utils may be extracted from react-dom in React v16.


Also, I dug way through the tree from react-test-renderer and found the important bits. I could extract this into a while (!found) {} loop, but I'm not sure if that would be any cleaner. At this point it is probably better to just grab enzyme for the nice API.

import renderer from 'react-test-renderer'
const wrapper = renderer.create(<NativeRouter><Link to='/'><Text>text</Text></Link></NativeRouter>)
const output = wrapper._component._instance._reactInternalInstance._renderedComponent._instance._reactInternalInstance._renderedComponent._instance._reactInternalInstance._renderedComponent._instance._reactInternalInstance._renderedComponent._instance._reactInternalInstance._renderedComponent._currentElement
output

{
  '$$typeof': Symbol(react.element),
  type: {
    [Function]
    displayName: 'TouchableHighlight',
  },
  key: null,
  ref: null,
  props: {
    to: '/',
    children: {
      '$$typeof': Symbol(react.element),
      type: [Function: Component],
      key: null,
      ref: null,
      props: [Object],
      _owner: null,
      _store: {}
    },
    replace: false,
    onPress: [Function],
    activeOpacity: 0.85,
    underlayColor: 'black'
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the other React Router packages, just use a <MemoryRouter> or create their own history object and pass it to a <Router>. This should do the same.

@@ -0,0 +1,85 @@
import React from 'react'
import ReactTestUtils from 'react-addons-test-utils'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to dredge this up, but this needs to be updated now that the react addons are deprecated. Should just be import ReactTestUtils from 'react-dom/test-utils'.

@bjax13
Copy link

bjax13 commented Apr 13, 2017

Excited for this to be implemented when it can be :D literally was trying to implement this in my project today. Is there an easy workaround for now? I need to fire a function when the link is pressed.

Previously, the Link component in react-router-native did not call
onPress if it was passed. This change makes it such that the handler
will be called if passed to the child component. This brings the
react-native-link behavior closer to that of react-router-dom/Link.
Preiously the Link in react-router-native did not obey any calls to
event.preventDefault(). This change makes it such that if the user calls
event.preventDefault() in their onPress handler, the Link will not
travel to the new location.
@timdorr
Copy link
Member

timdorr commented Apr 13, 2017

I'm not a RN user myself. Can anyone else comment on this and speak to its quality?

@bjax13
Copy link

bjax13 commented Apr 13, 2017

unfortunately I am new enough to development and RN that I would not be a good judge of quality. XD.

@bjax13
Copy link

bjax13 commented Apr 15, 2017

Timdorr I asked the RN subreddit to check it out and they said that it looked good. You can see there comments at this link. https://www.reddit.com/r/reactnative/comments/65ee43/could_someone_experienced_in_rn_review_this_code

@timdorr
Copy link
Member

timdorr commented Jul 5, 2017

OK, let's make this happen!

@timdorr timdorr merged commit 676e8ac into remix-run:master Jul 5, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants