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

eliminate React warnings re: api deprecation and increase test coverage #234

Merged
merged 3 commits into from Jun 24, 2015

Conversation

@dotMR
Copy link
Contributor

dotMR commented Jun 8, 2015

Hi,
When I was checking out your app I noticed some warnings in the console and here is a patch to fix them. Do let me know if there is a better option than targeting this branch - I don't know the codebase well so I wanted to include test coverage to validate that my changes weren't breaking anything, and the best place to accomplish that at the moment was in this branch (as I wasn't able to run tests effectively in master).

Commit Message
  • removed usage of 'transferPropsTo'
  • changed React.PropTypes.renderable -> React.PropTypes.node
  • removed unused property 'patientsComponent'
  • added test coverage for the changes

Issue: #228

dotMR added 2 commits Jun 8, 2015
- removed usage of 'transferPropsTo'
- changed React.PropTypes.component -> React.PropTypes.element
- changed React.PropTypes.renderable -> React.PropTypes.node
- removed unused property 'patientsComponent'
- added test coverage for the changes

Issue: #228
@jebeck
Copy link
Contributor

jebeck commented Jun 8, 2015

Before we can look at this, we need you to agree to our Volunteer/Contributer License Agreement, discussed here and available here. Please follow the instructions from the latter to add the appropriate comment to this pull request if you agree to the terms in the agreement. Thanks!

@dotMR
Copy link
Contributor Author

dotMR commented Jun 9, 2015

Thanks. I did see the contributor guidelines and included my agreement disclaimer in the pull request body above, but I'm happy to move it to a separate comment if that is the official place for it.

@dotMR
Copy link
Contributor Author

dotMR commented Jun 9, 2015

I agree to the terms of Tidepool Project’s Volunteer/Contributor License Agreement v1.0
as it exists at http://tidepool-org.github.io/TidepoolVCLA.pdf on Jun 8, 2015.

@jebeck
Copy link
Contributor

jebeck commented Jun 9, 2015

Weirdly, I can't see the VCLA agreement in the body of the PR here on GitHub, only in my e-mail?? Did you edit it? In any case, thanks for the agreement by way of comment.

Will be reviewing this likely later this week. Currently have other priorities and need to wait for some recent changes on master to go through QA. After that will be trying to pull in new-testing branch hopefully along with this. Will let you know if I have any questions/comments.

Thanks again!

@dotMR
Copy link
Contributor Author

dotMR commented Jun 9, 2015

No black magic going on, I updated the PR body to remove the disclaimer and added as a comment instead :)

Thanks for the update - no rush on my end.

@jebeck jebeck mentioned this pull request Jun 12, 2015
@@ -22,7 +22,6 @@ var personUtils = require('../../core/personutils');
var Invitation = React.createClass({
propTypes: {
invitation: React.PropTypes.object,
patientsComponent: React.PropTypes.component,

This comment has been minimized.

Copy link
@jebeck

jebeck Jun 12, 2015

Contributor

So I guess it looks like we're just not actually using this in the Invitation component. Can we remove where this prop is passed in too?

This comment has been minimized.

Copy link
@dotMR

dotMR Jun 15, 2015

Author Contributor

Yes, of course. I had intended to do that. Good catch!

@@ -160,7 +160,17 @@ var Patient = React.createClass({
},

renderPatientTeam: function() {
return this.transferPropsTo(<PatientTeam />);

This comment has been minimized.

Copy link
@jebeck

jebeck Jun 12, 2015

Contributor

All other things being equal, I'd prefer the JSX equivalent of transferPropsTo for this, which seems to be the recommendation post-deprecation of transferPropsTo: JSX Spread Attributes. Is there a reason you didn't think this was a good idea here @dotMR?

CC @GordyD because we've been talking about trying to do the JSX spread attributes in more places, potentially.

This comment has been minimized.

Copy link
@dotMR

dotMR Jun 12, 2015

Author Contributor

@jebeck I was actually on the fence. With that many props I see the argument that passing everything manually is tedious and verbose, but for me first looking at the code base I liked the explicitness of seeing exactly what was being passed. It also made it easier to try to find potential orphaned props. But I'm happy to change to spread if that is the desire going forward. They make quick mention of it in the docs here: Manually Transferring Props

it('should return an object with editing set to false', function() {
console.warn = sinon.spy();
// TODO: consider requiring Proptype: theNote (maybe define as a shape as well?)
// -> Properties of theNote are accessed in componentDidMount()

This comment has been minimized.

Copy link
@jebeck

jebeck Jun 12, 2015

Contributor

Feel free to address this TODO in this PR. Generally we do prefer isRequired on our props, but clearly there's some inconsistency across the codebase... Here I think we should be OK (i.e., without any other code changes) adding isRequired on theNote and onSaveEdit; possibly also on imageSize.

This comment has been minimized.

Copy link
@dotMR

dotMR Jun 15, 2015

Author Contributor

The way it is currently coded I don't believe imageSize or onSaveEdit are required, but I will make the update for theNote

- replaced manual prop transfer with JSX spread in Patient
- required theNote prop on Message, added test case
- removed passing of unused patientsComponent prop to Invitation
@jebeck
Copy link
Contributor

jebeck commented Jun 24, 2015

Thanks again for this! Merging :)

jebeck added a commit that referenced this pull request Jun 24, 2015
eliminate React warnings re: api deprecation and increase test coverage
@jebeck jebeck merged commit 0dd5cac into tidepool-org:new-testing Jun 24, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.