Preview menu: Remove redundant "opens in a new tab" hidden text

[kellychoffman]

Fixes: #24386

Text link already states that the link will open in a new tab, so this PR removes the visually hidden "(opens in a new tab)" text. (And yes, I realize this very description is redundant if you've read the title)

[jasmussen]

Before:

After:

Congrats on your first PR!

The test failure looks legitimate, I'll take a look and see if I can fix it.

[jasmussen]

It looks like the snapshots just needed updating. Usually whenever the markup of the UI changes, these need updating. I do it on the commandline using npm run test-unit -- --updateSnapshot, but I'm always happy to take a look.

[talldan]

It looks like there are a couple of other uses of PostPreviewButton, like in the PostLockedModal, and I think that only says 'Preview'.

An option could be to change the text everywhere to 'Preview in new tab', or make the VisuallyHidden text something that can be enabled/disabled through a prop..

[jasmussen]

Great catch, Probably risky to change that phrasing because it will likely not scale to mobile interfaces with the length of the phrase.

So we should probably restore the hint for that use case.

[afercia]

Yep, this was, in a way, mentioned on the issue:

Looking at the implementation, not sure what is the preferred approach. PostPreviewButton accepts a textContent prop and fallbacks to a link text Preview if none is passed. So the default text does need the visually hidden text, while a more meaningful text passed via textContent may not need it. Since this depends on the actual text passed via the prop, I'd tend to think the behavior can't be automated and maybe an additional, explicit, prop to render the visually hidden text would be the best option.

[kellychoffman]

It looks like the snapshots just needed updating

Thanks! TIL.

[jasmussen]

I tried a thing in 34a2d98, to simply make the VisuallyHidden component part of the same condition that outputs the short verb. @talldan can you sanity check it?

[talldan]

@jasmussen Looks good! I'm not sure what that change in the unrelated snapshot file is though:
packages/dependency-extraction-webpack-plugin/test/snapshots/build.js.snap

[jasmussen]

Thanks for looking. That's curious indeed, I didn't even notice it. I'll try a good rebase and regenerate the snaps afterwards. Happy to know the fix is okay though 👍

[jasmussen]

I rebased, which removed the snapshots discrepancy.

I am however seeing a test failure locally, both on this branch and in master, that seems unrelated to this PR but might nonetheless cause tests to fail here:

Summary of all failing tests
 FAIL  packages/components/src/tooltip/test/index.js
  ● Tooltip › #render() › should ignore mouseenter on disabled elements

    expect(jest.fn()).not.toHaveBeenCalled()

    Expected number of calls: 0

[jasmussen]

Hooray, tests are passing!

[jasmussen]

Congrats on your first PR here, Kelly, and thank you!

[github-actions]

Congratulations on your first merged pull request, @kellychoffman! 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!