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

External link 'Use goals to measure success' CTA opens in same window #3683

Closed
wpdarren opened this issue Jul 7, 2021 · 6 comments
Closed
Labels
Good First Issue Good first issue for new engineers Module: Analytics Google Analytics module related issues P2 Low priority Type: Bug Something isn't working
Milestone

Comments

@wpdarren
Copy link
Collaborator

wpdarren commented Jul 7, 2021

Bug Description

On the Site Kit Dashboard, when you have Google Analytics connected there is a Use goals to measure success CTA, and the Create a New Goal link does not open up in a new window like other external links.

image

Steps to reproduce

  1. Connect Google Analytics for a site with no goals created
  2. Go to the Site Kit dashboard
  3. Scroll down to the Use goals to measure success CTA
  4. Click on the link and see it opens in same window.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The CTA to "Create a new goal" from the Analytics goals widget should open in a new window/tab
    • In this case there should be no additional visual indicator since it already has the CTA arrow icon

Implementation Brief

  • in assets/js/components/legacy-notifications/cta.js implement ctaLinkExternal prop like it's already implemented on assets/js/components/SourceLink.js as external prop.
  • in the same component, add hideExternalIndicator prop to it's Link component (Line 54) if the value of ctaLinkExternal is truthy. ie hideExternalIndicator={ ctaLinkExternal }.
  • In assets/js/modules/analytics/components/dashboard/DashboardGoalsWidget.js Line 137, add the newly implemented ctaLinkExternalprop.

Test Coverage

  • No new tests required

Visual Regression Changes

  • No need to update since there should be no visual change.

QA Brief

  • Make sure that clicking the CTA button of Use goals to measure success in Analytics Module opens the link in new tab.

Changelog entry

  • Update Analytics goals widget CTA link to open in a new window.
@wpdarren wpdarren added the Type: Bug Something isn't working label Jul 7, 2021
@aaemnnosttv aaemnnosttv added Module: Analytics Google Analytics module related issues P2 Low priority labels Jul 13, 2021
@aaemnnosttv aaemnnosttv added the Good First Issue Good first issue for new engineers label Sep 1, 2021
@kuasha420 kuasha420 self-assigned this Sep 1, 2021
@kuasha420 kuasha420 removed their assignment Sep 1, 2021
@eugene-manuilov
Copy link
Collaborator

@kuasha420 IB mostly looks good, but let's name the new property to be ctaLinkExternal for consistency with other ctaLink and ctaLabel properties. Also, please, enter Test Coverage and Visual Regression Changes section as well.

@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov
Copy link
Collaborator

@kuasha420 we also need to add the hideExternalIndicator property to the Link element in the CTA component to satisfy the following AC:

In this case there should be no additional visual indicator since it already has the CTA arrow icon

Could you please update the IB?

@eugene-manuilov
Copy link
Collaborator

eugene-manuilov commented Sep 3, 2021

  • in assets/js/components/legacy-notifications/cta.js implement ctaLinkExternal prop like it's already implemented on assets/js/components/SourceLink.js as external prop.
  • in the same component, add hideExternalIndicator prop as well so no external marker css is outputted.
  • In assets/js/modules/analytics/components/dashboard/DashboardGoalsWidget.js Line 137, add the newly implemented ctaLinkExternal & hideExternalIndicator prop.

@kuasha420 we don't need to add the hideExternalIndicator property to the CTA component itself, we need to add it to the Link element if the ctaLinkExternal property is truthy. So, it means that we don't need to set hideExternalIndicator in the DashboardGoalsWidget component.

@eugene-manuilov
Copy link
Collaborator

IB ✔️ Thanks, @kuasha420!

@eugene-manuilov eugene-manuilov removed their assignment Sep 3, 2021
@kuasha420 kuasha420 self-assigned this Sep 3, 2021
@kuasha420 kuasha420 removed their assignment Sep 3, 2021
@aaemnnosttv aaemnnosttv self-assigned this Sep 6, 2021
@aaemnnosttv aaemnnosttv removed their assignment Sep 6, 2021
@eclarke1 eclarke1 added this to the Sprint 57 milestone Sep 6, 2021
@wpdarren wpdarren self-assigned this Sep 6, 2021
@wpdarren
Copy link
Collaborator Author

wpdarren commented Sep 6, 2021

QA Update: ✅

Verified that when clicking the CTA button of Use goals to measure success in Analytics Module opens the link in new tab.

image

@wpdarren wpdarren removed their assignment Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: Analytics Google Analytics module related issues P2 Low priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants