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: Direct About Skate clicks to the /user-guide endpoint #2593

Merged
merged 2 commits into from
May 14, 2024

Conversation

joshlarson
Copy link
Contributor

Copy link

Coverage of commit ae2c5dd

Summary coverage rate:
  lines......: 93.7% (3232 of 3448 lines)
  functions..: 73.4% (1337 of 1822 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson marked this pull request as ready for review May 10, 2024 22:07
@joshlarson joshlarson requested a review from a team as a code owner May 10, 2024 22:07
@joshlarson joshlarson force-pushed the jdl/feat/about-link-to-user-guide branch from ae2c5dd to cd30730 Compare May 10, 2024 22:27
Copy link

Coverage of commit cd30730

Summary coverage rate:
  lines......: 93.7% (3232 of 3448 lines)
  functions..: 73.4% (1337 of 1822 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Comment on lines 181 to 185
<NavLink
className={({ isActive }) =>
"c-left-nav__link" +
(isActive ? " c-left-nav__link--active" : "")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: This doesn't go anywhere within the app, so I'm wondering, what benefits do we get from using <NavLink> here over an <a>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm - probably not much benefit, huh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-05-13 at 1 42 43 PM

Flawless victory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sooo I think we gain some benefit in consistency. Even if we don't need all of the features of NavLink, we still get the benefit of it effortlessly looking like the other links in this section!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assets/tests/components/nav/leftNav.test.tsx Show resolved Hide resolved
assets/tests/components/nav/navMenu.test.tsx Show resolved Hide resolved
Copy link

Coverage of commit 3768de7

Summary coverage rate:
  lines......: 93.7% (3232 of 3448 lines)
  functions..: 73.4% (1337 of 1822 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson enabled auto-merge (squash) May 14, 2024 15:00
@joshlarson joshlarson merged commit 237d74a into main May 14, 2024
33 checks passed
@joshlarson joshlarson deleted the jdl/feat/about-link-to-user-guide branch May 14, 2024 15:02
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.

2 participants