Add React and React-router #6
Conversation
package.json
Outdated
| "type": "git", | ||
| "url": "git+https://github.com/CMU-17-356/dronuts-2019-group-5.git" | ||
| "version": "0.1.0", | ||
| "private": true, |
There was a problem hiding this comment.
Why did we change the repo to be private?
There was a problem hiding this comment.
This is entirely because I deleted old package.json to run CRA. Good catch!
src/tests/App.test.tsx
Outdated
| @@ -0,0 +1,9 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Looks like we moved test directory from root to src in this PR - this is probably wise: http://www.wisdomofjim.com/blog/why-i-recommend-unit-tests-in-the-src-folder just want to make sure this was intentionally done...
There was a problem hiding this comment.
This was not a conscious decision, just me following different directory structures in different tutorials at different points in time. I think it's fairly arbitrary, though, as long as we stick to one.
tslint.json
Outdated
| "no-null-keyword": true, | ||
| "prefer-const": true, | ||
| "jsdoc-format": true | ||
| "extends": [], |
There was a problem hiding this comment.
I don't understand what happened to linting here... it looks like we removed the core functionality - maybe this is added in later commit? Or will become clearer in later commit? WIll check back...
There was a problem hiding this comment.
I think this is actually an artifact of me messing around with manually putting all the parts together (before I just went to using create-react-app). CRA itself doesn't support typescript (link) right now, so I'm going to remove this. Linting is provided by ESLint instead.
| @@ -1,3 +1,7 @@ | |||
| # Because otherwise Travis tests will fail | |||
| # Todo: look into this | |||
| package-lock.json | |||
There was a problem hiding this comment.
I couldn't comment on your prior commit as it as deleting a file (package-lock.json) - but It's unclear to me why deleting this file was necessary? Rather than regenerating it.. trying to understand your thinking -- would love to hear your thoughts!
There was a problem hiding this comment.
When package-lock.json is present, Travis runs npm ci rather than npm install (link). This causes tests to fail because dependencies are missing.
I suspect this is because devDependencies are not installed, but I don't know how to fix that. I also don't know how to disable the use of npm ci except by omitting package-lock.json.
| import React, { Component } from 'react'; | ||
| import logo from './logo.svg'; | ||
| import './App.css'; | ||
| import * as React from 'react'; |
There was a problem hiding this comment.
I'm still going through React tutorial - I'll return after I've finished tutorial to review - unless others beat me to it..
|
I just pulled this PR branch locally and got it up n' running w/ docker. I'll finish the React tutorial and then return to finish the review (specifically, on the last commit of this PR), which is all React additions. |
- Remove redundant tslint.json and update README about linting - Restore package.json details from old version
|
Ok - I've reviewed your React code as well as tested locally. All looks good - although I have some questions about routing in general, which hopefully we can spend ~5 minutes on when you arrive for group meeting - if that's OK with you @bobbiec. Merging this branch. |
|
Oh wait, can you update your branch w/ latest master locally, and then push? It's a few commits behind. @bobbiec And then I'll merge. Thanks. |
…nto create-react-app
|
When we rerun each of these Travis builds independently, they work. Going to merge to master! |
This PR adds React with react-router.
I basically just deleted everything we had before and ran create-react-app, and then added react router and a little sample code.