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

Add boolean prop, 'openInNewTab' to KExternalLink #137

Merged
merged 2 commits into from
Jan 5, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Dec 1, 2020

Summary:

  • Added boolean prop openInNewTab to KExternalLink
  • Added :target to anchor tag in <template> to open up new tab if openInNewTab is given true

Fixes #94

Testing:

  • Tested with Chrome on MacOSX, but not yet with other versions of Kolibri.
    Screen-Recording-2020-11-30-at-7 28 34-PM

@sairina sairina requested a review from nucleogenesis December 1, 2020 03:46
@indirectlylit
Copy link
Contributor

This looks good, thanks!

It wasn't in the scope of the original issue, but I'd like to suggest that while we're doing this work, we include an iconographic representation of the "new tab" behavior. We do something similar in the design system itself:

image

thoughts?

@nucleogenesis
Copy link
Member

nucleogenesis commented Dec 2, 2020

@indirectlylit There is an issue for that #123 -

One thing not noted in that issue is that we want it to be conditional upon the new openInNewTab prop to be shown.

@sairina maybe we could add the icon. You'll find the name of the icon here: https://kolibri-design-system.netlify.app/icons/#icons that you'll want to pass to the :icon prop of <KIcon>

Additionally - we'll want to make sure that it displays with the correct color and on the right side of the link (just like in #123) - but when we're RTL it should show on the left side.

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.

We'll want to also add rel="noopener noreferrer" attr to that <a> tag for security purposes

https://mathiasbynens.github.io/rel-noopener/ has a great breakdown of why we need to add this to external links.

@sairina
Copy link
Contributor Author

sairina commented Dec 4, 2020

@nucleogenesis Thanks for that explanation! I'll add that in.
@indirectlylit I'll work on adding the icon!

@nucleogenesis nucleogenesis merged commit 3e76a9a 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 ought to open in new tab safely
3 participants