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

fix: Moved "Return to Regular Route" into root directions list #2521

Merged
merged 11 commits into from
Mar 29, 2024

Conversation

hannahpurcell
Copy link
Collaborator

@hannahpurcell hannahpurcell commented Mar 26, 2024

The little things I like about this approach is that

  • "Return to Regular Route" is put with other directions, which I think it should be!
  • The direction is in the root hook instead of scattered in multiple components
  • The test for diversionPage now sufficiently covers the check "was 'Return to Regular Route' added to the directions sequence"
  • We can still choose to make directions more structured with typing later (per Josh's suggestion)

Also included, updating the Storybook story for this!

@hannahpurcell hannahpurcell requested a review from a team as a code owner March 26, 2024 18:36
@hannahpurcell hannahpurcell changed the title Hp/return to route update fix: Moved "Return to Regular Route" into root directions list Mar 26, 2024
Copy link

Coverage of commit fc1d9aa

Summary coverage rate:
  lines......: 94.2% (3211 of 3407 lines)
  functions..: 73.7% (1323 of 1795 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link

Coverage of commit fc1d9aa

Summary coverage rate:
  lines......: 94.3% (3212 of 3407 lines)
  functions..: 73.8% (1324 of 1795 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link

Coverage of commit 2bfae55

Summary coverage rate:
  lines......: 94.3% (3212 of 3407 lines)
  functions..: 73.8% (1324 of 1795 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

firestack
firestack previously approved these changes Mar 28, 2024
Copy link
Member

@firestack firestack left a comment

Choose a reason for hiding this comment

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

Two comments but otherwise this looks good!

@firestack firestack dismissed their stale review March 28, 2024 14:50

didn't mean to approve just yet 😬 and I would like those comments resolved before merging

Co-authored-by: Kayla Firestack <[email protected]>
@joshlarson
Copy link
Contributor

I think it looks good - I made a few non-blocking nits, but the only reason I'm not approving is so that I don't scoop Kayla's review.

Copy link

Coverage of commit fb5fea8

Summary coverage rate:
  lines......: 94.2% (3211 of 3407 lines)
  functions..: 73.7% (1323 of 1795 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link
Member

@firestack firestack left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @hannahpurcell!!

assets/tests/components/detours/diversionPage.test.tsx Outdated Show resolved Hide resolved
@hannahpurcell hannahpurcell merged commit fc5df73 into main Mar 29, 2024
12 checks passed
@hannahpurcell hannahpurcell deleted the hp/return-to-route-update branch March 29, 2024 20:09
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