-
Notifications
You must be signed in to change notification settings - Fork 182
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
#433 improved fetch results to 10 #436
#433 improved fetch results to 10 #436
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request addresses issue #433 by modifying the getSearchResults function in js/zap-common.js to increase the number of fetched results to 10. This change involves adding a single line of code to set the maxResults parameter to 10. Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @RLalor - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -6,6 +6,7 @@ function getSearchResults(query, youTubeDataApiKey, onData, onFail) { | |||
key: youTubeDataApiKey, | |||
part: "snippet", | |||
q: query, | |||
maxResults: 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider making maxResults configurable
Hardcoding the maxResults to 10 might limit the flexibility of the function. Consider making it a parameter so that the caller can specify the desired number of results.
maxResults: 10, | |
function fetchYouTubeData(query, maxResults = 10) { | |
$.get("https://www.googleapis.com/youtube/v3/search", { | |
key: youTubeDataApiKey, | |
part: "snippet", | |
q: query, | |
maxResults: maxResults, | |
type: "video" | |
}, onData).fail(onFail); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @RLalor!
Thanks. This is a nice project to work on. The other changes you mention look quite easy as well. If I get bored I will play with and test those as well and discuss which option is most suited. |
Description
added 1 line of code to increase results to 10 as requested in #433 until one of the other options is implemented.
Checklist
Summary by Sourcery
Increased the maximum number of fetch results to 10 in the YouTube search query.