feat: Link Component #161 #309
Conversation
ahobson
requested review from
gidjin,
haworku,
sarboc,
suzubara and
tinyels
as code owners
ahobson
force-pushed
the
adh-link-component-ideas
branch
from
9ca5fe5
to
0351060
Compare
haworku
changed the title
src/components/Link/Link.tsx
Outdated
|
|
||
| type LinkProps = { | ||
| href: string | ||
| children: React.ReactNode |
There was a problem hiding this comment.
Added these back. Trying to follow the pattern of requiring props that are fundamental to the element.
Without these props <Link className="usa-button" variant="unstyled" /> is valid code. It compiles without warning ... and a regular anchor tag in our app would warn about this for missing href and children (since we use jsx-a11y to check valid anchor tags).
src/components/Link/Link.tsx
Outdated
| ) | ||
| } | ||
|
|
||
| export function Link<FCProps = LinkProps>( |
There was a problem hiding this comment.
I think we still need overloads to get accurate intellisense and type suggestions. What I notice is right now when the asCustom is not present and Link has invalid props, it only warns about not including asCustom, maybe because it has the broadest types. It doesn't warn for anchor tag.
Here's what an overload before line 46 could look (I think):
// Overloads
export function Link<T>(asCustomProps: AsCustomProps<T>): React.ReactElement
export function Link(linkProps: LinkProps): React.ReactElement
Lmk if you want to pair on this or if I'm off base.
There was a problem hiding this comment.
Thank you for this suggestion. I'll have to look into this. I think I will have time on Friday
There was a problem hiding this comment.
Thank you! I think I've simplified the type signatures and added the overloads. It seems to do what we expect in VSCode now.
There was a problem hiding this comment.
LGTM - made a couple comments more for documentation reasons.
I would still love to see us get the typescript intellisense actually working. This could be a model for other components in the future of how to use these flexible types but still have decent type safety.
But if it feels to others like overloads aren't a good idea -- let's set this aside for now and move on! Thanks for figuring out the generic types at least that's a win!
src/components/Link/Link.stories.tsx
Outdated
| /* | ||
| * TODO: sort out how to test visited links in storybook | ||
| * | ||
| * export const Visited = (): React.ReactElement => ( |
There was a problem hiding this comment.
Looking at the USWDS source (https://designsystem.digital.gov/components/typography/#links) I think they just added the usa-color-text-visited class to their docs theme for demonstration purposes. We probably could do the same in a Storybook-only CSS file (these can be added in ./storybook):
.usa-color-text-visited {
color: color($theme-link-visited-color);
}
There was a problem hiding this comment.
we may also want to do something similar to the Default story so that it never gets the :visited style applied. IMO adding CSS to Storybook to enforce the intended styles for demonstration purposes is a 💯 valid thing to do.
|
|
||
| export const StyledAsButton = (): React.ReactElement => ( | ||
| <p> | ||
| <Link className="usa-button" variant="unstyled" href={'#'}> |
There was a problem hiding this comment.
suggestion for future iteration: I have a feeling the usa-button and all of its variations will be a common use-case for Link components, so adding the possible button style/className props to the Link component might be a good idea.
haworku
force-pushed
the
adh-link-component-ideas
branch
from
301381a
to
a90c3d5
Compare
Summary
Adds new Link component. Includes handling for
asCustomprop.Related Issues or PRs
relates to #161
How To Test