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

Fix the withRef mess #3740

Merged
merged 1 commit into from
Aug 19, 2016
Merged

Fix the withRef mess #3740

merged 1 commit into from
Aug 19, 2016

Conversation

taion
Copy link
Contributor

@taion taion commented Aug 19, 2016

Cleans up code style, variable naming, and tests

@taion taion force-pushed the fix-withRef branch 2 times, most recently from 57184e5 to b11c1fd Compare August 19, 2016 03:40
@timdorr
Copy link
Member

timdorr commented Aug 19, 2016

Beat you to it :P

@timdorr timdorr closed this Aug 19, 2016
@taion
Copy link
Contributor Author

taion commented Aug 19, 2016

There were a few more things to clean up in the test code, including how e.g. https://github.com/reactjs/react-router/blob/master/modules/__tests__/withRouter-test.js#L77-L78 calls expect after calling done.

I'll rebase and merge.


function getDisplayName(WrappedComponent) {
return WrappedComponent.displayName || WrappedComponent.name || 'Component'
}

export default function withRouter(WrappedComponent, options) {
const { withRef } = options || {}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather do destructuring in case the options grow. If they won't ever (I dunno, maybe?) then this should just be a simple boolean arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure deal with that when we get there, and for now skip creating an empty object that doesn't get any use.

@taion
Copy link
Contributor Author

taion commented Aug 19, 2016

Updated to simplify the render branch a bit

@taion
Copy link
Contributor Author

taion commented Aug 19, 2016

Gah okay tests should actually pass now

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.

2 participants