-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: Add nav variant to Link component #846
Conversation
…ing yarn start in the example app triggering an auto-update of the typescript config
@@ -40,16 +40,17 @@ function linkClasses<T>( | |||
): string | undefined { | |||
const unstyled = variant === 'unstyled' | |||
const isExternalLink = variant === 'external' | |||
const isNavLink = variant === 'nav' | |||
|
|||
return unstyled | |||
? className |
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.
The way the classnames package works is it already conditionally includes the css. This means we don't need to add another ternary here. Check out the linked docs if you've never used this package before, its pretty convenient.
We should be able to get this still passing tests but following a pattern that's more similar to this pseudocodey example:
return unstyled?
className // if its unstyled, only use classes passed in by props
:
classnames( // otherwise use...
'usa-link', // any classes required for component
{ 'random-class': true}, // and any conditionally included CSS
className // and also add the classes passed in by props
)
We add className
last as a convention in this library because we always want the styles that come in through props to be applied on top of other styles. There are several examples of this throughout react-uswds codebase to check out too if you want.
Swap with |
@suzubara do we use Happo differently with this project than on MM? |
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 looks great!
One question to double check on the change to the tsconfig.json
, otherwise I'm good to approve!
Congrats on your first react-uswds PR 💯
Summary
Add a
nav
option to thevariant
property to style the existingLink
component with classNameusa-nav__link
Related Issues or PRs
#166
How To Test
yarn test
yarn storybook
Viewable from Typography > Link > Nav Link
Screenshots (optional)
Storybook