Conversation
| import '../styles/app.scss'; | ||
|
|
||
| class App extends React.Component { | ||
| constructor() { |
There was a problem hiding this comment.
Probably not an issue here as you're not using a Router, but its a good idea to pass props into your constructor and super methods.
| country: '', | ||
| }, | ||
| }; | ||
| this.handleForecastSelect = this.handleForecastSelect.bind(this); |
There was a problem hiding this comment.
You can do this, but it's easier to define your methods as arrow functions so they bind their own scope.
src/components/app.jsx
Outdated
| } | ||
|
|
||
| componentDidMount() { | ||
| const url = 'https://mcr-codes-weather.herokuapp.com/forecast?city=Bolton'; |
There was a problem hiding this comment.
To make things easier for other developers, we tend to put constants like this into config files. That way anything that need to be customised can all be found in one place.
| return ( | ||
| <React.Fragment> | ||
| <LocationDetails | ||
| city={this.state.location.city} |
There was a problem hiding this comment.
This might be a good place for a bit of destructuring. eg:
const { city, country } = this.state.location;
It's a good way to save tying out this.state 100 times.
There was a problem hiding this comment.
You can then access the values just by using city and country.
| description: PropTypes.string.isRequired, | ||
| icon: PropTypes.string.isRequired, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Not the end of the world, as this is a personal project, but it would be worth making sure you add propTypes to the rest of your components.
|
|
||
| LocationDetails.propTypes = { | ||
| city: PropTypes.string.isRequired, | ||
|
|
There was a problem hiding this comment.
I'd imagine EsLint really doesn't like this blank line here.
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const LocationDetails = props => <h1 className="location-details">{props.city}, {props.country}</h1>; |
There was a problem hiding this comment.
Its perfectly valid to use an implicit return like this, but it would be a lot more readable to move your return value down to a new line.
| import ForecastSummary from '../../src/components/forecast-summary'; | ||
|
|
||
| it('renders the date', () => { | ||
| const wrapper = shallow(( |
There was a problem hiding this comment.
You can still use the beforeEach hooks with enzyme. In this case it would stop you having to shallow render the same component over and over again.
| date="mockDate" | ||
|
|
||
| temperature="mockTemperature" | ||
|
|
There was a problem hiding this comment.
Some odd line spacing here...
ian-antking
left a comment
ian-antking
left a comment
There was a problem hiding this comment.
Some really good work here!.
I've added a few comments on how you could get some advanced techniques into you code.
At some point you should change the name of your repository on github to better suit what the actual project is. Be sure to edit your package.json with new author information as well.
If you do use the bootstrap again, you can change the folder it clones into by putting a new name after the url:
git clone git@github.com:MCRcodes/react-bootstrap.git weather-app
2 participants
No description provided.