[Issue #8845] Refactor returnToGrants link#8951
Conversation
| <div className="bg-primary-lightest line-height-body-2 font-body-3xs padding-y-1"> | ||
| <div className="grid-container"> | ||
| <SearchCallToAction /> | ||
| {t.rich("goToGG", { |
There was a problem hiding this comment.
This refactor implements new behavior in t.rich("goToGG") with both internal and external links, but I don’t see replacement coverage for this flow. Would be super helpful to add a test that asserts the banner renders and that the Grants.gov link includes the expected safe external-link attrs.
There was a problem hiding this comment.
Added minimal tests (tho there's no good way to test for external-link attrs in intl'd content).
| <div className="bg-primary-lightest line-height-body-2 font-body-3xs padding-y-1"> | ||
| <div className="grid-container"> | ||
| <SearchCallToAction /> | ||
| {t.rich("goToGG", { | ||
| "search-link": (chunks) => <Link href="/search">{chunks}</Link>, | ||
| "gg-link": (chunks) => ( | ||
| <a href="https://grants.gov/" target="_blank"> | ||
| {chunks} | ||
| </a> | ||
| ), | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
Nit: I could be wrong, but since this is an important banner-style message, have we thought about giving it alert semantics (for example a USWDS alert/site-alert pattern)? Right now it’s just plain text in a div, so screen reader folks may not get any extra signal that this is important context.
There was a problem hiding this comment.
Even though this is styled very similarly to the usa-alert component, I don't think it's an alert.
Do you think this needs to be an alert? Or is the problem that it's a new pattern that looks similar to an alert?
I don't see much value in using usa-alert , as we'd then have to override many of its styles (the left border, content width/container, font size, etc…). And the only thing it'd get us is an ARIA role, but the usa-alert doesn't require an ARIA role unless you wanna raise the importance of its message. If we do want to make this an alert, I could be convinced to add role="region". But our <SimplerAlert> component only has two roles: "error" or "alert". And this message doesn't rise to either of those levels of importance.
I don't see much value in refactoring our <SimplerAlert> component and overriding all its styles for this piece of content (which is a temporary solution btw). I see this as similar to the "Official website…" usa-banner, which does not have an ARIA role.
|
Totally makes sense to remove the old component and its tests, but we did drop all coverage around the “return to Grants.gov” messaging in the same PR. Let’s make sure we replace that with search-page-level tests so we still cover the user-facing path end to end. |
|
Moved the banner to a component and added tests. @btabaska I think I've addressed all your concerns. Re-review? |
Summary
Fixes #8845
Changes proposed
ReturnToGrantsNotificationcomponent and test filesSuspenseandReturnToGrantsNotificationfrom the<Breadcrumbs>componentneeds to sit beside it in the layout
utm_sourcequery parameter, which is no longer needed to conditionally show the "Return to GG" link/searchpage heading directly into the page'sh1SearchCallToActioncomponent (which was just theh1and "Return to GG" link)/searchheadingContext for reviewers
This PR is the first step in refactoring the Breadcrumbs component so that all pages are managing them consistently. Removing the "Return to GG" link from Breadcrumbs will make that easier.
Validation steps