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

Tahuana's assignment #6

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

tahuana
Copy link

@tahuana tahuana commented Feb 27, 2018

Task 1: basic + stretch goals
Task 2: basic goal

class Recipe extends Component {
constructor(props){
super(props)
}

Choose a reason for hiding this comment

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

You don't need this constructor

Choose a reason for hiding this comment

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

You can also make this stateless! (see my comment on your error component)

class Error extends Component {
constructor(props){
super(props)
}

Choose a reason for hiding this comment

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

Same with this constructor! You don't need this :)

src/App.js Outdated

componentDidMount() {
const q = 'cakes'
const endpoint = `${Credentials.URL}?app_id=${Credentials.APP_ID}&app_key=${Credentials.APP_KEY}&q=${q}`

Choose a reason for hiding this comment

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

nice!

</div>
)
}
}

Choose a reason for hiding this comment

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

since you're not manipulating state or using any of the lifecycle hooks, you can make this a stateless component. Rather than extending the Component class, you can do something like this:

export const Error = (props) =>
    <div>
    {
        console.log("estou no erro")
    }

        <p>Ops... we got an error: {props.message}</p>
    </div>

export default Error;

You can try doing the same with your recipe component as well!

src/App.js Outdated
const endpoint = `${Credentials.URL}?app_id=${Credentials.APP_ID}&app_key=${Credentials.APP_KEY}&q=${q}`
//const endpoint = `${Credentials.URL}?app_id=${Credentials.APP_ID}&app_key=${Credentials.APP_KEY}`

const fetchRecipes = () =>

Choose a reason for hiding this comment

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

Consider pulling this function outside of the scope of componentDidMount

Something like this

 componentDidMount() {
  this.fetchRescipe()
}
fetchRecipes = () => <your fancy code here>

@tahuana
Copy link
Author

tahuana commented Mar 5, 2018

Done:

  • refactored based on TA comments
  • Task 2: double stretch goal - created a higher order component that conditionally show either an error or a list
  • Task 3: stretch goal - created a reusable function that if given a string, will search the api for that string
  • Task 4: double stretch goal - implemented a search bar

src/App.js Outdated
@@ -6,6 +6,9 @@ import Recipe from './components/Recipe.js'
import Error from './components/Error.js'
import SearchBar from './components/SearchBar'


const branch = (test, ComponentOnPass, ComponentOnFail) => props => test ? props.errorMessage.map(error => <Error message={error} />) : props.recipeList.map(recipe => <Recipe myRecipe = {recipe} />)

Choose a reason for hiding this comment

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

It looks like you've got the concepts of a HOC down! Nice work 😄 - this is a really complicated concept!

One thing you could do to make this more generic is pass your props directly to your pass and fail components instead of manipulating the props here, similar to this:

const branch = (test, ComponentOnPass, ComponentOnFail) => props => 
test 
? < ComponentOnPass data={props} />
: <ComponentOnFail data={props} />

Then you can use the data however you'd like within the component itself, which means you could pass any components to your branch component and reuse your branch component multiple times.

One other thing to double check is that you're passing your parameters into your HOC in the order you expect them - when you call this on line 67 you're passing 1) your condition, 2) your success component (Recipe) and 3) your fail component (Error). Here, you're rendering your Error component as the success component (which I think is is what you actually want to render, since you're checking for hasError). It could be worth renaming your test boolean something like passCondition or similar so it's clear that the first returned component is the one you want rendered if your condition is true. And if you want your Recipe component to show with the success of your condition, you could change your boolean to !this.state.hasError.

Excellent work with Higher Order Components!

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.

3 participants