Components: Update ExternalLink to support onClick handler

[Initsogar]

What?

Fixes #45212

The idea is to make <ExternalLink> component to handle onClick events again, based on the issue #45212. Also it includes some unit testing for this component.

Why?

onClick handler on <ExternalLink> component is currently being used in multiple instances in Jetpack monorepo to record tracking events, and we consider it is necessary to include custom logic on that handler.

This stopped working after a change implemented for validating anchor links in #42259.

How?

We are creating a new function for the onClick handler to:

• Validate and execute onClick whether it is passed as a prop.
• Validate if the link is an internal anchor, so we can prevent default action to not impact what was implemented on #42259

Testing Instructions

You can run unit tests for this:
npm run test:unit -- external-link/test/index.tsx

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Initsogar! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[alexstine]

@Initsogar Thanks for your PR. I do like how the code is a little cleaner now and makes more sense.

One thing that will be needed, the changelog will need updating at packages/components/changelog.md. Probably call this a bug fix if it is currently not working.

[ciampo]

@chad1008 , would you be able to review this PR ?

[chad1008]

Changes to the component look good to me 🙂. I've included some inline comments on the new unit tests.

Other than that I think all this will need is a CHANGELOG entry!

[chad1008]

With the suggested change to userEvent, you'll need a different test of the defaultPrevented property. Maybe something like:

const onClickMock = jest.fn();

// ...

expect( onClickMock ).toHaveBeenCalledTimes( 1 );
expect( onClickMock ).toHaveBeenLastCalledWith(
  expect.objectContaining( {
    defaultPrevented: true
  } )
);

[Initsogar]

@chad1008 The idea of this specific test case is to validate it is properly preventing default action if it is an anchor link, without passing onClick prop to our component (I will update the test case name). I'm trying to find a way to accomplish this using userEvent, but don't find any yet. Do you have any idea how we could handle it?

[Initsogar]

I think we can mock the onClick after rendering the link from the component and validate we are receiving the defaultPrevented: true. Something like:

const user = setupUser();

render(
	<ExternalLink href="#test">I&apos;m an anchor link!</ExternalLink>
);

const link = screen.getByRole( 'link', {
	name: "I'm an anchor link! (opens in a new tab)",
} );

const onClickMock = jest.fn();
link.onclick = onClickMock;

await user.click( link );
expect( onClickMock ).toHaveBeenCalledTimes( 1 );
expect( onClickMock ).toHaveBeenLastCalledWith(
	expect.objectContaining( { defaultPrevented: true } )
);

What do you think?

[Initsogar]

I tested this approach, and it is working as expected. I will push new changes so you can review it properly :)

[chad1008]

Ah, yes I see your point... hard to test the behavior when no onClick is passed by ...passing a mock to onClick 😆

Your proposed solution looks like works, although modifying the event listener imperatively is an unusual pattern. Maybe we just add a brief comment to those two tests explaining that we're using that approach specifically so we can test the defaultPrevented without passing an onClick prop? It's described in the test title, but a comment closer to the actual link.onclick modification might help make it crystal clear for future maintainers.

[Initsogar]

Correct! 🥲 I just added the comments!

[Initsogar]

Thanks @alexstine and @chad1008, I will be working on the suggested changes!

[Initsogar]

Hmm, I clicked to ask for re-review from @chad1008 and @alexstine and somehow it messed up and removed the other ones that were on the list. Sorry 😅

I just added the changelog entry and made changes on the unit test related to use getByRole and userEvent instead of getByTestId and fireEvent/createEvent. I would appreciate your review :)

[Initsogar]

@chad1008 I just added some comments for clarification purposes on the approach used for unit testing defaultPrevented on this component while no passing onClick prop. Also resolved the conflict with components CHANGELOG.md.

[chad1008]

Looks great @Initsogar, thank you! I left one last inline comment (sorry for not spotting that last time), but after that I think we're good to go 🚀 🚢

Congrats on your first core contribution, btw! 🎉

[chad1008]

Just noticed this data-testid is still here, but no longer needed. Looks like there's another lingering in the next test as well. We can remove both before merging 🙂

[Initsogar]

You are right! Thank you for that!
Just fixed it :)

[Initsogar]

@chad1008 @ciampo @mirka Hey there! I pushed the latest suggestion and I think we are ready. Could you confirm and let me know what would be the next step? :) Thanks for all your support!

[ciampo]

Hey @Initsogar , I re-ran the failing tests (it looks like they're being a flakey lately) and they passed!

I'll go ahead and merge your changes — thank you for helping the project, and congrats on your first contribution 🎉 Happy to collaborate more in the future in case you felt like opening more PRs like this one

[github-actions]

Congratulations on your first merged pull request, @Initsogar! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!