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

Update <KExternalLink> with icon and noopener noreferrer attr #142

Merged
merged 13 commits into from
Jan 5, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Dec 24, 2020

Summary

Background: openInNewTab attr was recently added to <KExternalLink> so that a value of true would allow for the link to be opened in another tab. Previously, <KExternalLink> used target="_blank" and did not have an icon to differentiate it as an external link as opposed to a link to another URL internally.

In this PR

  1. The noopener noreferrer attr was also added as an extra security measure to ensure window.opener = null because target="_blank" is used in the component.
  2. The "open in new tab" icon was added.

Issues:

Addresses #123
Addresses #137
Addresses #94

Expected Behavior

Use of this component should:

  1. Allow for an external link to be opened safely in a new tab
  2. The "open in new tab" icon should open correctly depending on the language - see below.
    -- to the Left of the link when link is written in RTL languages (e.g. Arabic)
    -- to the Right of the link when link is written in LTR languages (e.g. English)

RTL/LTR Screenshots

Icon on Left for Arabic link (Arabic) Icon on Right for English link (English)
Screen Shot 2020-12-23 at 10 22 53 PM Screen Shot 2020-12-23 at 10 23 12 PM
Icon on Right for English link (Arabic) Icon on Right for English link (English)
Screen Shot 2020-12-23 at 10 23 57 PM Screen Shot 2020-12-23 at 10 23 34 PM

Follow-up Notes

@indirectlylit
Copy link
Contributor

Looking great, and thanks for testing the RTL behaviors!

Two small notes:

  • Should probably have a few more pixels of margin between the icon and the text so it feels less cramped
  • When inline with an RTL-language link, the icon should be flipped so it points up and to the left. This can be achieved by adding rtlFlip: true to iconDefinitions.js

@sairina
Copy link
Contributor Author

sairina commented Dec 25, 2020

Updated screenshots:

Margin added (Arabic) Margin added (English)
Screen Shot 2020-12-24 at 8 35 23 PM Screen Shot 2020-12-24 at 8 35 42 PM

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

This is looking good except one thing I noted. I tested on Studio as well and it looked great! Thanks @sairina

lib/buttons-and-links/KExternalLink.vue Show resolved Hide resolved
@sairina
Copy link
Contributor Author

sairina commented Jan 4, 2021

Updated so that margin no longer shows when there is no icon: this.icon was not defined in my code, so I used this.openInNewTab to check for whether or not the icon existed (since we only have the icon when openInNewTab is true.

Arabic (No icon) English (No icon)
Screen Shot 2021-01-04 at 2 49 34 PM Screen Shot 2021-01-04 at 2 49 59 PM
Arabic (With icon) English (With icon)
Screen Shot 2021-01-04 at 2 48 44 PM Screen Shot 2021-01-04 at 2 50 27 PM

@sairina
Copy link
Contributor Author

sairina commented Jan 4, 2021

@nucleogenesis In Arabic, for the Update Notification modal (UpdateNotification.vue), the external link icon appears on the left side of the link (as opposed to the right side in, say, the privacy modal) and an extra margin has been created. Nvm - issue fixed.

Update Modal w/RTL-lang link Previous
Screen Shot 2021-01-04 at 10 29 14 PM Screen Shot 2021-01-04 at 2 51 20 PM

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Great work! Thanks @sairina !

@nucleogenesis nucleogenesis merged commit 72a29c4 into learningequality:v0.2.x Jan 5, 2021
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.

KExternalLink should have an open_in_new icon KExternalLink ought to open in new tab safely
3 participants