Skip to content

Code Review - #3

Open
josemussa wants to merge 1 commit into
masterfrom
cr-branch
Open

Code Review#3
josemussa wants to merge 1 commit into
masterfrom
cr-branch

Conversation

@josemussa

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src-cr/App.css
margin: auto;
}

body html {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For more readability we can place body and html rules on top of your main css file, since it's kind of the 1st/principal rule.

Comment thread src-cr/App.js

import PlacesContainer from "./Containers/PlacesContainer"

class App extends Component {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

When you're not using any lifecycle methods, ie. componentDidMount constructor, or your component is stateless, there's no need to write it using the class syntax, this is because react will render the return of any pure function.

So we can rewrite this as follows:

import React from 'react' // Note that we're not importing Component anymore, since it's not needed.

cosnt App = () => (
  <div className="App">
     ...
   </div>
);

export default App;

Comment thread src-cr/App.test.js
import ReactDOM from 'react-dom';
import App from './App';

it('renders without crashing', () => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We can test the App component using snapshots.

import React from 'react';
import App from './App';
import renderer from 'react-test-renderer';

it('renders correctly', () => {
  const appWrapper = renderer
    .create(<App />)
    .toJSON();
  expect(appWrapper).toMatchSnapshot();
});


class Event extends React.Component{

state = {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

👍


fetchEvents = (nextProps) => (

fetch(`https://cors-anywhere.herokuapp.com/https://api.yelp.com/v3/events?limit=4&radius=2&location=${nextProps.zip}&start_date=1523880000&end_date=1525089600&categories=music,visual-arts,fashion,food-and-drink,festivals-fairs,kids-family`, {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

When building a real app you'll need to do a lot of API calls, writing this code multiple times seems kinda wrong, I recommend you to create a utility function to wrap this and pass the url as an argument:

export const apiFetcher = (url) => fetch(url, { header: { Authorization: `Bearer ${process.env.REACT_APP_YELP_API_KEY}` }});

Then on your component you can do

import { apiFetcher } from './utils/apiFetcher';

// And you use it as

fetchEvents = (nextProps) => {
  apiFetcher('https://cors-blablabla').then(...)
} 


handleResponse = (place) => {
this.setState({
place: place

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

When using ES6 as you're doing, you don't need to specify the key of an object if the const has the same name.

this.setState({ place }) // equals: { place: valueOfPlaceVariable }


return(
<React.Fragment>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You can deconstruct the state to have more readibility.

const { place } = state;

<img src={place.card_photo} classNa... />




export default PlaceCard

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Always add PropTypes to your components https://reactjs.org/docs/typechecking-with-proptypes.html

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

PlaceCard.propTypes = {
  id: PropTypes.number,
  name. PropTypes.string,
}

<HeroImage/>
<br/>
<select name="select" onChange={this.props.handleChange} className="ui fluid normal dropdown">
<option value="all">All</option>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You can try out placing all this values on an array and mapping them.

const categories = ['sightseeing', 'discovering'];

// Then on your render method

<select>
  {categories.map(category => <option value={category}>upperFirst(category)</option>)}
</select>

https://lodash.com/docs/4.17.5#upperFirst

filterCategory = () => {
if(this.state.selectedCategory === "all"){
return this.state.places
}else{

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No need for this else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant