Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Began working on Search functionality #2

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

Conversation

rdrnt
Copy link

@rdrnt rdrnt commented Feb 24, 2018

Hi! I really like the work that you've done.

I'm a bit new to contributing to OS, so sorry if you're only supposed to PR if you've fully completed the work. I began working on search functionality, its not close to 100% complete, but I'd love to create a branch for it and others can help work on it there.

Thanks!

[FIX]: Search screen now displays searches
* @return {Promise} Returns a promise
*/

const searchPost = (options = defaultSearchOptions, query='') => {
Copy link
Owner

Choose a reason for hiding this comment

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

Where does defaultSearchOptions come from?

const searchOptions = {
sortByDate: false || options.sortByDate,
pageNumber: 1 || options.pageNumber,
tags: 'story' || options.tag,
Copy link
Owner

Choose a reason for hiding this comment

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

For these variables see my comments below:

const searchOptions = {
  sortByDate: false || options.sortByDate, // will always be options.sortByDate or undefined as false || <anything> will always be <anything>
  pageNumber: 1 || options.pageNumber, // will always be 1
  tags: 'story' || options.tag, // will always be 'story`
}

Better to do change the condition for all three, e.g: sortByDate: options.sortByDate || false

const queryParameter = `query=${query}`;
const tagParameter = `&tags=${searchOptions.tags}`

const searchUrl = `${config.apiSearch}${dateParameter}?${queryParameter}${hitsPerPage}${tagParameter}`;
Copy link
Owner

Choose a reason for hiding this comment

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

Change config. apiSearch to config.search once updated in config/default.json

@@ -1,5 +1,6 @@
{
"api": "https://hacker-news.firebaseio.com/v0",
"apiSearch": "http://hn.algolia.com/api/v1/",
Copy link
Owner

Choose a reason for hiding this comment

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

Can we rename apiSearch to search to fit in with api and base

@lukakerr
Copy link
Owner

Thanks for the contribution! I left a few comments. Are you planning on adding anymore? If not, I can merge after you've made the changes, otherwise i'll leave the PR open for a while longer so you can work on it

[FIX]: Fixed searchOptions conditions

[CHORE]: Added yarn.lock to .gitignore
@rdrnt
Copy link
Author

rdrnt commented Feb 24, 2018

Hi! I changed those things 👍

I plan on contributing a lot more, so we'll keep this open 💃

[CHORE]: Added packagelock.json to .gitignore
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants