Open all external links in a new tab

[sandesh-sp]

Closes #873

[netlify]

✅ Deploy Preview for veda-ui ready!

Name	Link
🔨 Latest commit	58c89ec
🔍 Latest deploy log	https://app.netlify.com/sites/veda-ui/deploys/6605c50b63a8cc00084c4776
😎 Deploy Preview	https://deploy-preview-870--veda-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[slesaad]

@sandesh-sp i tried the deploy preview link and this external link opened in the same tab 👀

[sandesh-sp]

@slesaad I didn't consider the external links on the cards but only within the stories. Let me look at that right away.

[sandesh-sp]

@slesaad External links present on cards should also open in a new tab now

[j08lue]

Thanks for the contribution, @sandesh-sp! Please always reference the ticket you are working off of.

I guess it was this one:

• All external links should open in a new tab US-GHG-Center/veda-config-ghg#315

But I had not considered that we would change this centrally in VEDA UI, so I now created a ticket in this repo for it:

• Add option to open all external links in new tabs #873

As I stated in the ticket, we need to clarify: Would it be very complex to make this behavior optional?

[slesaad]

@j08lue @sandrahoang686 yes, we do want to make the behavior optional. @sandesh-sp is looking at making it configurable via the config file in the the veda-config repo probably via the https://github.com/NASA-IMPACT/veda-config/blob/develop/veda.config.js file or an env var?

[j08lue]

via the veda.config.js file or an env var?

For the E&A feature flag, we chose the .env file, like here.

But I'll defer to @sandrahoang686 to advise.

[sandrahoang686]

@slesaad @sandesh-sp I would recommend to make this type of configuration in the veda.config.js file since we use that file to specify overrideable behavior to things like stylistic changes already. Also usually env vars are typically related at the deployment level and the deployment environment while I think of configuration files at the code level and storing info in relation to the app, in this case, instance.

[sandesh-sp]

Creating a new pull request in veda-config-ghg
US-GHG-Center/veda-config-ghg#326

[j08lue]

I see you closed this pull request, @sandesh-sp. Is the change in VEDA UI not needed?

[slesaad]

@j08lue I think that was a mistake. Reopening. @sandrahoang686 @hanbyul-here can you guys have a look at the approach followed here and let us know what you think?

[sandrahoang686]

@slesaad @sandesh-sp this looks like a better path, thanks for making the updates! Could we possibly move this defining of linkProps into a more global util file? I think app/scripts/utils/url.ts should be fine. Then maybe update the method to work for those three cases.. something like below possibly:

const getLinkProps = (
  isExternalLink: string[] | boolean,
  linkTo: string,
  as?: React.ForwardRefExoticComponent<LinkProps & React.RefAttributes<HTMLAnchorElement>>,
  onClick?: () => void
) => {
  const externalLinksInNewTab = getBoolean('externalLinksInNewTab');
  return isExternalLink
  ? {
      href: linkTo,
      ...(externalLinksInNewTab
        ? {target: '_blank', rel: 'noopener noreferrer'}
        : {}),
      ...(onClick ? {onClick: onClick} : {})
    }
  : {
      ...(as ? {as: as} : {}),
      to: linkTo,
      ...(onClick ? {onClick: onClick} : {})
    };
}

For SmartLink it would then look like...

export default function SmartLink(props: SmartLinkProps) {
  const { to, ...rest } = props;
  const isExternalLink = /^https?:\/\//.test(to);
  const linkProps = getLinkProps(isExternalLink, to);
  return isExternalLink ? (
    externalLinksInNewTab ? (
      <a {...linkProps} {...rest} />
  ) : (
    <Link {...linkProps} {...rest} />
  );
}

[sandesh-sp]

Thanks @sandrahoang686 ! I'll make the changes as suggested

[sandesh-sp]

@sandrahoang686 could you have a look to the new changes. I defined getLinkProps on utils/url which was then used to get the needed attributes for the links.
ts-check is throwing an error around the type of onClick argument (currently defined as MouseEventHandler) and I'm not sure what its value should be.

const getLinkProps = (
  isExternalLink: string[] | boolean,
  linkTo: string,
  as?: React.ForwardRefExoticComponent<LinkProps & React.RefAttributes<HTMLAnchorElement>>,
  onClick?: MouseEventHandler
)

[sandrahoang686]

@sandesh-sp what is your exact error? Maybe try typing onClick with onClick?: (() => void) | MouseEventHandler;.

Another error looks like Link is expecting the to prop and in this case it looks like typescript is not catching on since it does look like your code would return that prop if !isExternalLink.. a solution might be to check that linkProps does return to or type the return?

[sandesh-sp]

Thanks @sandrahoang686, this seems to be working. Could you please review it.

[sandrahoang686]

Will this work? I tested in the deploy preview and looks like the external link I clicked wasn't opening in a new tab?

[sandesh-sp]

@sandrahoang686 that's because we also need externalLinksInNewTab set to true on veda-config/veda.config.js.
It is working on veda-config-ghg (US-GHG-Center/veda-config-ghg#326) and also on my local machine.
I had to set it this way to prevent typescript error which was expecting "to" prop from the function

[sandrahoang686]

lgtm

[sandesh-sp]

@j08lue @slesaad Could you please review the changes?

[sandrahoang686]

@sandesh-sp letting you know Jonas is out and should be back monday !

[j08lue]

Referring to @sandrahoang686 for an authoritative review of the technical implementation.

From a feature perspective, this seems complete now with the option to enable / disable external links to open in new tabs per-instance.