-
Notifications
You must be signed in to change notification settings - Fork 12
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
va-link: Replacing external icon with text #1396
Conversation
@@ -285,8 +285,7 @@ export class VaLink { | |||
onClick={handleClick} | |||
target="_blank" | |||
> | |||
{text} | |||
<va-icon class="external-link-icon" icon="launch"></va-icon> | |||
{text} (opens in a new tab) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have flagged a visual change in Chromatic on the va-link
and va-statement-of-truth stories at a minimum. Can you try toggling the onlyChanged
flag to see if that picks it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check what's up with va-privacy-agreement
? It doesn't look like it's updated: https://3303-external-link-update--65a6e2ed2314f7b8f98609d8.chromatic.com/?path=/docs/uswds-va-privacy-agreement--docs
Also is it possible to keep the period from dropping to the next line in va-statement-of-truth
?
@jamigibbs Looks like the privacy agreement wasn't using the va-link component, just a hard-coded link and va-icon. I'll update it, thanks for catching that! I'll see what I can do about the statement of truth dropped period, that period exists outside of the va-link component so I'll have to mod the va-statement-of-truth component someway somehow. |
<va-icon class="hydrated privacy-policy-icon"></va-icon> | ||
<span class="usa-sr-only">opens in a new window</span> | ||
</a>. | ||
<va-link class="hydrated"></va-link>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised this test doesn't expect more details on the link. 🤔 Like these attributes:
href="/privacy-policy/"
text="privacy policy"
external
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was surprised by that too. Good thing we still have separate e2e tests for the va-link component itself to make sure those props work!
@@ -16,6 +16,12 @@ | |||
margin: 24px; | |||
} | |||
|
|||
@media screen and (max-width: $small-screen) { | |||
:host .short-line { | |||
width: var(--vads-size-line-length-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see this new token coming in handy!
Chromatic
https://3303-external-link-update--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
Closes #3303
QA Checklist
Screenshots
Acceptance criteria
Definition of done