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.

Sign up for GitHub

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

Jump to bottom

Add rel noopener noreferrer to target _blank links #319

Merged
merged 2 commits into from
Jul 19, 2018
Merged

Add rel noopener noreferrer to target _blank links #319

merged 2 commits into from
Jul 19, 2018

Conversation

anselmbradford
Copy link
Member

Changes

  • Adds rel="nopener noreferrer" to two PDF links that were missing that with target="_blank" (reported by @lfatty).
  • Normalizes quotes to double quotes.
  • Removes some extraneous whitespace.

Testing

@coveralls
Copy link

coveralls commented Jul 19, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling e21faea on target_blank_links into a75769f on master.

marteki
marteki approved these changes Jul 19, 2018
View reviewed changes
Copy link
Member

@marteki marteki left a comment

Choose a reason for hiding this comment

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

We've been trying to get the proper attributes put on the target="_blank" links for a long time. Clearly, we missed some of them somehow.

There are changes made to files in this repo that aren't being pushed to the live site, but I think that's fine in order to show what we should be doing everywhere for links in new tabs.

@marteki marteki mentioned this pull request Jul 19, 2018
Merged
chosak
chosak reviewed Jul 19, 2018
View reviewed changes
Copy link
Member

@chosak chosak left a comment

Choose a reason for hiding this comment

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

This LGTM.

Do we want to also add noreferrer to the links generated by JavaScript, like here and here and here and here?

Also, I notice this comment -- and I wasn't able to get this file working locally -- but does it make sense to update the links in it, like here?

paying_for_college/templates/repay_student_debt.html Outdated
@@ -1110,7 +1110,7 @@ <h4 class="sample-text-header"><span class="cf-icon cf-icon-email"></span> Sampl
<div class="content-l_col content-l_col-1-4 tip-section">
<div class="tip_bar"></div><div class="tip_line"></div>
<h3>TIP</h3>
<p>Remember, if you’re having a problem with a student loan , you can submit a complaint <a href="http://www.consumerfinance.gov/complaint/" class="internal-link" target="_blank">online</a> or call us at (855) 411-2372.</p>
<p>Remember, if you’re having a problem with a student loan , you can submit a complaint <a href="http://www.consumerfinance.gov/complaint/" class="internal-link" target="_blank" rel="noopener noreferrer">online</a> or call us at (855) 411-2372.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Minor fix: remove extra space in "student loan , you can submit".

Copy link
Member

@marteki marteki Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If we make that fix here, we should also actually fix it in django-college-costs-comparison. This file is never deployed to the website from this repo, but comes from the other repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this here at least

@anselmbradford
Copy link
Member Author

anselmbradford commented Jul 19, 2018 •
edited
Loading

This LGTM.
Do we want to also add noreferrer to the links generated by JavaScript, like here and here and here and here?
Also, I notice this comment -- and I wasn't able to get this file working locally -- but does it make sense to update the links in it, like here?

Updated all of these. THanks!

@anselmbradford anselmbradford merged commit 205f92b into master Jul 19, 2018
@anselmbradford anselmbradford deleted the target_blank_links branch July 19, 2018 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chosak chosak chosak left review comments

@marteki marteki marteki approved these changes

@higs4281 higs4281 higs4281 approved these changes

@mistergone mistergone Awaiting requested review from mistergone

@willbarton willbarton Awaiting requested review from willbarton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anselmbradford @coveralls @higs4281 @chosak @marteki