-
Notifications
You must be signed in to change notification settings - Fork 972
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
[Improvement] Adding noreferrer on doc links #1709
Conversation
Signed-off-by: manasvis <[email protected]>
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.
This is a solid implementation of linked issue.
However, in reviewing, this felt like the kind of thing we shouldn't have to manually worry about for every link. I did a little digging, and it turns out that neither noopeneer
nor noreferrer
need to be explicitly set when using <EuiLink>
components with target="_blank"
.
Here is where that EUI component calculates the ref
values:
https://github.com/elastic/eui/blob/1a5e8429c2db1c331946aede3f85e144fa21d381/src/components/link/link.tsx#L163
noreferrer
will always be set by https://github.com/elastic/eui/blob/1a5e8429c2db1c331946aede3f85e144fa21d381/src/services/security/get_secure_rel_for_target.ts#L40-L42
and noopener
will be set by
https://github.com/elastic/eui/blob/1a5e8429c2db1c331946aede3f85e144fa21d381/src/services/security/get_secure_rel_for_target.ts#L44-L46
So my recommendation is to update the changes to the 4 <EuiLink>
components to remove the rel
prop altogether, rather than manually adding noreferrer
. (The current changes to <a>
strings seem worthwhile to keep)
Codecov Report
@@ Coverage Diff @@
## main #1709 +/- ##
=======================================
Coverage 67.51% 67.51%
=======================================
Files 3067 3067
Lines 58985 58985
Branches 8944 8944
=======================================
+ Hits 39821 39825 +4
+ Misses 16985 16982 -3
+ Partials 2179 2178 -1
Continue to review full report at Codecov.
|
Thanks Josh for the pointers! Looks like |
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.
LGTM
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.
LGTM, thanks for the updates!
Signed-off-by: manasvis <[email protected]> (cherry picked from commit 3ff99cf)
Signed-off-by: manasvis <[email protected]> (cherry picked from commit 3ff99cf) Co-authored-by: Manasvini B Suryanarayana <[email protected]>
Signed-off-by: manasvis <[email protected]>
…project#1727) Signed-off-by: manasvis <[email protected]> (cherry picked from commit 3ff99cf) Co-authored-by: Manasvini B Suryanarayana <[email protected]>
…project#1727) Signed-off-by: manasvis <[email protected]> (cherry picked from commit 3ff99cf) Co-authored-by: Manasvini B Suryanarayana <[email protected]>
Signed-off-by: manasvis [email protected]
Description
Stemming from this PR: #565. Links inconsistently have noreferrer on the reference.
This change might not include adding noreferrer to all the links in the package, but links which already have 'rel="noopener"' in it to make it consistent throughout the repo as called out in the above PR.
Issues Resolved
#567
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr