Skip to content
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

Merged
merged 8 commits into from
Feb 5, 2021

Conversation

SirenaBorracha
Copy link
Contributor

@SirenaBorracha SirenaBorracha commented Feb 3, 2021

Summary

Add a nav option to the variant property to style the existing Link component with className usa-nav__link

Related Issues or PRs

#166

How To Test

yarn test

yarn storybook
Viewable from Typography > Link > Nav Link

Screenshots (optional)

Storybook
Screen Shot 2021-02-03 at 2 49 27 PM

@@ -40,16 +40,17 @@ function linkClasses<T>(
): string | undefined {
const unstyled = variant === 'unstyled'
const isExternalLink = variant === 'external'
const isNavLink = variant === 'nav'

return unstyled
? className
Copy link
Contributor

@haworku haworku Feb 4, 2021

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.

@haworku
Copy link
Contributor

haworku commented Feb 4, 2021

Swap with usa-link__nav

@SirenaBorracha SirenaBorracha changed the title [WIP][feat]: Add nav variant to Link component feat: Add nav variant to Link component Feb 4, 2021
@SirenaBorracha SirenaBorracha marked this pull request as ready for review February 4, 2021 23:01
@SirenaBorracha
Copy link
Contributor Author

@suzubara do we use Happo differently with this project than on MM?

Copy link
Contributor

@brandonlenz brandonlenz left a 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 💯

example/tsconfig.json Show resolved Hide resolved
@SirenaBorracha SirenaBorracha merged commit c92159e into main Feb 5, 2021
@SirenaBorracha SirenaBorracha deleted the ak-feat-add-nav-variant-to-link-166 branch February 5, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants