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

fix #129 -external links #131

Merged
merged 2 commits into from
Jul 5, 2016
Merged

fix #129 -external links #131

merged 2 commits into from
Jul 5, 2016

Conversation

marcobiedermann
Copy link
Collaborator

Only open external links in a new tab

@marcobiedermann
Copy link
Collaborator Author

@joshbuchea @scottaohara Can we merge this in?

@scottaohara
Copy link
Collaborator

scottaohara commented Jul 2, 2016

If we add this, could you also make sure to add rel="noopener noreferrer" to the off-site links as well? more information about target="blank" security issues here

@joshbuchea
Copy link
Owner

👍 looks good to me

@joshbuchea
Copy link
Owner

What do you think @scottaohara?

@scottaohara scottaohara merged commit fd0a375 into joshbuchea:gh-pages Jul 5, 2016
@tomlutzenberger
Copy link
Contributor

On Line 38 in _layouts/default.html, you initialize var i and do it again in the for-Loop.
I would change it either to remove Line 38 and say for(var i = 0; or leave the first "parameter" in the for-Loop empty.

@marcobiedermann
Copy link
Collaborator Author

@tomlutzenberger Sorry, my bad. I actually wanted to declare it outside the for loop and assign a value inside the loop to it.
I do not recommend your suggestion: for (var i = 0; …).

JavaScript hoists variables to the top of the current scope.
Declaring the variable first and assign a value later is slightly more performant. You actually would not notice any difference but I am a fan of more efficient code. Lots of small things add up to a bigger impact :)

@marcobiedermann
Copy link
Collaborator Author

@tomlutzenberger fixed in 19e4127

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.

4 participants