-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Limit issues displayed on contributor page #1290
Conversation
also -- what should the limit be? 5 seemed good to me, but perhaps that's a little low. |
added filtering of beginner issues out from the "help wanted" issues list also changed callbacks to filter/limit data via lodash -- i think this might be more readable? let me know if tweaks need to be made to better fit code style here though :) |
scripts/contributing.js
Outdated
} | ||
var limitedData = _.take(data, ISSUES_LIMIT); | ||
|
||
_.forEach(limitedData, function(issue) { |
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.
What's the advantage of _.forEach
over limitedData.forEach
?
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.
hm, i suppose none. at least, nothing intentional. i see no reason to use lodash over it -- i'll change it
scripts/contributing.js
Outdated
for(var i = 0; i < data.length; i++){ | ||
$('.open-issues-section--beginner .issues').append('<li class="issue"><a href="' + data[i].html_url + '"><span class="issue__number">#' + data[i].number +'</span><span class="issue__title">' + data[i].title + '</span></a></li>'); | ||
} | ||
var limitedData = _.take(data, ISSUES_LIMIT); |
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.
data.slice(0, ISSUES_LIMIT)
scripts/contributing.js
Outdated
var limitedData = _.take(data, ISSUES_LIMIT); | ||
|
||
_.forEach(limitedData, function(issue) { | ||
$('.open-issues-section--beginner .issues').append('<li class="issue"><a href="' + issue.html_url + '"><span class="issue__number">#' + issue.number +'</span><span class="issue__title">' + issue.title + '</span></a></li>'); |
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.
I know this is similar to how it was before, but ideally we should remove the XSS vulnerabilities from this 😛
We have jQuery available here, so this should not be building HTML elements like this:
<span class="issue__title">' + issue.title + '</span>
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.
great point! i'll change that here
Deploy preview ready! Built with commit 1dccffa |
gonna close this -- it's sat for a while and is trivial enough to be implemented elsewhere easily. feel free to reopen if needed though! |
Hey Noah, would totally like to add something like this again for the new site and if we could find a way to link a label to a page that would be cool |
Added feature mentioned in #1192 (comment)
(will gladly take on the other bullet point later too!)