search

woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

WC vs. WP external `<Link>`s #984

Open tomalec opened 3 years ago

tomalec commented 3 years ago

Describe the bug:

I noticed that for external documentation links we use @woocommerce/components.Link with external prop, and occasionally add <ExternalIcon>, then manually tweak it, to look a tad better.

I wonder:

  1. Why don't we use @wordpress/components.ExternalLink?
  2. Why WC one does not attach the icon as the WP one?
    1. That question/or feature request could be propagated to wc-admin if we don't know.
  3. Was that a conscious decision to make <ExternalIcon> non-clickable in unsupported countries notice?

image WordPress <ExternalLink> on left, WooCommerce > GLA <AppDocumentationLink /><ExternalIcon /> to the right.

Personally, I find the left one more appealing, plus it reduces the style tweaks on our end and just gets it for free from WP. Also, (however without deeper review of all cases), as a user, I, like it when the UI constantly visually marks all external links, so I know, when to expect to be navigated away.

@j111q, @eason9487 Do you have any comments on that?

Additional notes:

  1. Context: asking as I'm about to add another external link in notice for https://github.com/woocommerce/google-listings-and-ads/issues/363#issuecomment-879669042
  2. Figma link for reference https://www.figma.com/file/jZUpa8eTrnrK1Lwt2ry7zk/Google-Listings-%26-Ads-v1.0?node-id=3058%3A573
eason9487 commented 3 years ago

TIL: there's a @wordpress/components.ExternalLink component. 👍 Looks like it's suitable than composing them on our own if wrapping the <ExternalIcon> in the clickable link is fine.

Also, (however without deeper review of all cases), as a user, I, like it when the UI constantly visually marks all external links, so I know, when to expect to be navigated away.

👍 upvote for this.

j111q commented 3 years ago

I don't think it was a conscious choice, your suggestion sounds good! 👍 Thanks for checking 😄

jconroy commented 3 years ago

@ecgan probably has more background but I think there might be have been a gotcha or some preference to do with tracks events

ecgan commented 3 years ago

TIL for me too, didn't notice the @wordpress/components.ExternalLink component. 👍

  1. Why don't we use @wordpress/components.ExternalLink?

I think that within us WooCommerce ecosystem, we should use the @woocommerce/components's Link component. Currently, inside the component, there isn't really any code specific for type="external". However, just in case in the future there are component code change for it, our plugin would be ready for it (we just need to update the component package, hopefully).

Why WC one does not attach the icon as the WP one?

  1. That question/or feature request could be propagated to wc-admin if we don't know.

It has always been that way, as far as I know. Maybe that was how WooCommerce wanted it.

We can probably try to implement the icon in @woocommerce/components's Link component in WC Admin repo. If that's not possible, we can do that within our own repo.

3. Was that a conscious decision to make <ExternalIcon> non-clickable in unsupported countries notice?

Yeah I agree that the left one is better. The text and icon has the same color, and the click target is bigger.

as a user, I, like it when the UI constantly visually marks all external links, so I know, when to expect to be navigated away.

I agree too. 👍