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

move from predefined arrays to ajax calls to api #35

Open
atherdon opened this issue May 17, 2018 · 18 comments
Open

move from predefined arrays to ajax calls to api #35

atherdon opened this issue May 17, 2018 · 18 comments

Comments

@atherdon
Copy link
Collaborator

also similar to react native app recipe logic

@nickisaacs
Copy link

Can I pick this up? Please provide me details if you are okay with it

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 1, 2018

it's a not an easy task - do you think you can handle it? @nickisaacs
Btw, just yesterday i finished a first part of documentation, related to our search API server, so for a first part of this task - i got you covered. basically, @chauhannishith make some code, related to API calls, so you'll not need to start from scratch. hope this information will help you

@nickisaacs
Copy link

I do this for a living, so yes :)
Would love to help you out

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 1, 2018

Ok, @nickisaacs good to know about it! I'll be happy to help with any of your questions. and will try to speed up your development process, by giving as much information as i can.
So here is the current version of documentation, related to our search api server. it's not finished, so if you find something strange - just avoid it :)
https://chickenkyiv.gitbook.io/documentation/search-api-tests

@nickisaacs
Copy link

@atherdon
I can see you are currently reading from the JS files and populating the data inside the render method of each component which is not okay. Ideally, the data should be populated inside componentDidMount(). You will face problems when you switch to ajax calls. I plan to refactor this. Is that fine?

I went through your documentation quickly. I could see this: https://chickenkyiv.gitbook.io/documentation/search-api-tests/simple-queries/ingredients-by-name
You have mentioned the URLs dont work. So If I were to begin right now, where should I start?

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 2, 2018 via email

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 2, 2018 via email

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 2, 2018 via email

@nickisaacs
Copy link

@atherdon
I've implemented it for all selects except for ingredients. I can't find an endpoint/query that gives me a list of all the ingredients.
BTW, im fetching the data from inside the component now. Do you want me to fetch it from the parent and then pass it down as props to the children? Or maybe we can start using redux for this?

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 4, 2018

I think you should check this page for ingredient endpoints: https://chickenkyiv.gitbook.io/documentation/search-api-tests/ingredients

In short, you should check this URL for the list of ingredients:
https://loopback-recipe-search.herokuapp.com/api/ingredient

Maybe we should use React Content API instead of Redux?
If fetching data at parent will reduce logic in our form fields and will keep them more clean - i'm ok with that.

Q: Can you add id attribute to each option, so when we'll send a search request to a server -> we'll pass an array of ids?

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 4, 2018

@nickisaacs
Copy link

nickisaacs commented Jul 4, 2018

@atherdon
Ok, will use the new context API for this.
Yes, fetching data before rendering the selects should be better in my opinion, especially since you have components like cuisines which are shown two times, and call the same API twice currently. Sure, I will add the id as a prop to the Option component, currently I was using it as the key.

Would it be possible to have an API that returns data for all the selects in one network call? Does that sound like a good idea to you?
Something like {allergies: [...], cuisines: [...], ... }

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 4, 2018 via email

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 4, 2018 via email

@nickisaacs
Copy link

@atherdon
I have completed this activity, but it seems another commit was already made to fetch data. I have merge conflicts right now. Should I proceed with this?

@chauhannishith
Copy link
Contributor

@nickisaacs you can undo the commit and make it available on a different branch. That way he will have both options to compare and choose

@nickisaacs
Copy link

@chauhannishith Ok done :)
@atherdon Here is the PR: #73
My branch is here: https://github.com/nickisaacs/recipe-search-react/tree/feature/async-calls-select

PS - My eslint autoformatted all the files I edited. I hope that's not going to be a problem

@atherdon
Copy link
Collaborator Author

atherdon commented Jul 6, 2018

@nickisaacs i'm working on updates, related to form right now. will review your changes when finish my part.

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

No branches or pull requests

3 participants