feat: Programmatically determine when links should open in new tabs #1100
Conversation
meissadia
requested review from
billhimmelsbach and
tanner-ricks
as code owners
meissadia
force-pushed
the
when-to-open-links-in-new-tab
branch
from
9287a04 to
b96809e
Compare
meissadia
force-pushed
the
when-to-open-links-in-new-tab
branch
from
b96809e to
e2c6403
Compare
…L <Link> component requires auth data
There was a problem hiding this comment.
Awesome work here @meissadia! Code looks good, I just added a few minor comments. Once Natalia gives her approval, we're good to go.
| }; | ||
|
|
||
| /** | ||
| * Tests - Verify the state of the Filing homepage at each stage of the Filing process |
There was a problem hiding this comment.
Very thorough @meissadia and I super appreciate checking out all these card links!
Could you do me a favor, and while we're here, can we confirm the heading text on the card for these different states? Good way of confirming you're in the state you're expecting for each of these cards, and I'd be great to have some coverage on that.
There was a problem hiding this comment.
Validations added: commit
| import { expectedPaperworkReductionActUrl } from '../../../utils/testFixture.utils'; | ||
| import { SelectorLinkText, selectLink } from '../../../utils/verifyLinkTargets'; | ||
|
|
||
| test('Unauthenticated homepage: Paperwork Reduction Act', async ({ page }) => { | ||
| await test.step('Verify on the Paperwork Reduction Act and link exists', async () => { |
There was a problem hiding this comment.
Nice little update here.
NIT: Could we change the title to this to begin with "Verify link" or something just so I can run all these tests as a batch when I search for them?
| @@ -46,7 +48,54 @@ export function IconExternalLink(): ReactElement { | |||
| ); | |||
| } | |||
|
|
|||
| export function isNewTabImplied(href: string | undefined): boolean { | |||
| // Determine if the the target href should open in a new tab | |||
| export function useIsNewTabImplied(href: string | undefined): boolean { | |||
There was a problem hiding this comment.
NIT: Could you do me another favor here and just add like a 2 line comment explaining why we're doing it this way for our future selves? Something like, determines if a link should be opened in a new tab based on design team's rules to avoid bespoke logic per link?
… motivation for the useIsNewTabImplied hook
Closes #1071
Changes
useIsNewTabImpliedthat determines when links should open in new tabsopenLink,openLinkNewTab)expectLinkOpensSameTab,expectLinkOpensNewTab)Follow-up ticket?
I did a refactor of E2E tests to address my concerns about:
However there is still inconsistency in
test("Verify link targets", ...))It is probably easiest to identify where/how to make these improvements here in the same PR. At the same time, I feel like the tests we have implemented do a solid job of verifying the functionality and would identify any breaking changes, so are probably "good enough as-is".