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: Detour map not interactive until route pattern selected #2662

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

hannahpurcell
Copy link
Collaborator

@hannahpurcell hannahpurcell requested a review from a team as a code owner June 14, 2024 19:33
Copy link

Coverage of commit acef487

Summary coverage rate:
  lines......: 93.6% (3257 of 3480 lines)
  functions..: 73.4% (1343 of 1830 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack
Copy link
Member

Looks good here! One question, do you think it'd be worth trying to add tests for this? I didn't see any similar tests for the share panel, so I'm not sure we have anything existing to build off of?

joshlarson
joshlarson previously approved these changes Jun 14, 2024
Copy link
Contributor

@joshlarson joshlarson left a comment

Choose a reason for hiding this comment

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

LGTM!

@joshlarson joshlarson dismissed their stale review June 14, 2024 20:31

Kayla's comment - I think tests are doable here

@joshlarson
Copy link
Contributor

@hannahpurcell I think you could assert on the lack of a .c-detour_map--original-route-shape selector (because that's the interactive bit - the core, non-interactive one is .c-detour_map--original-route-shape-core) after the Change route or direction button has been pressed.

@hannahpurcell
Copy link
Collaborator Author

Kayla's right in that we really didn't have enough existing scaffolding to easily add a test for this. I've spent an hour on it, tried making a state machine snapshot factory that was less than successful, and can only think to mock the state machine, which we should certainly do, but is also certainly overkill for the small change happening here. Thoughts?

@firestack
Copy link
Member

Kayla's right in that we really didn't have enough existing scaffolding to easily add a test for this. I've spent an hour on it, tried making a state machine snapshot factory that was less than successful, and can only think to mock the state machine, which we should certainly do, but is also certainly overkill for the small change happening here. Thoughts?

You may be able to take the "when a route pattern is already selected, opens to selected pattern" test and turn it into a test for this

    test("<rename this>", async () => {
      const route = routeFactory.build()
      const routePattern = routePatternFactory.build({
        routeId: route.id,
      })

      jest.mocked(fetchRoutePatterns).mockResolvedValue([routePattern])

      const { container } = render(
        <RoutesProvider routes={[route]}>
          <DiversionPage
            originalRoute={{ route, routePattern }}
          />
        </RoutesProvider>
      )

      await userEvent.click(
        screen.getByRole("button", { name: "Change route or direction" })
      )

      expect(
        container.querySelectorAll(".c-detour_map--original-route-shape-core")
      ).toHaveLength(<N>)

      expect(
      	container.querySelectorAll(".c-detour_map--original-route-shape")
      ).toHaveLength(<M>)
    })

The query selectors are a bit hard to read though, so it may be worth doing the abstraction where we can make the selectors within the expect a bit more readable to understand which one is actually not interactive. I think we have a ticket for this?

At the very least, should probably add comments for which one is interactive and which one isn't

@hannahpurcell
Copy link
Collaborator Author

You may be able to take the "when a route pattern is already selected, opens to selected pattern" test and turn it into a test for this

I'm OOO today but think this shouldn't be blocked from going out today by a test (given Kathleen's prioritization of the issue). If making the above change would be super smooth (taking <30min of effort / review), would you be interested / able to do that? Otherwise, do you feel ok with shipping, and I add the test on Thursday?

@firestack
Copy link
Member

You may be able to take the "when a route pattern is already selected, opens to selected pattern" test and turn it into a test for this

I'm OOO today but think this shouldn't be blocked from going out today by a test (given Kathleen's prioritization of the issue). If making the above change would be super smooth (taking <30min of effort / review), would you be interested / able to do that? Otherwise, do you feel ok with shipping, and I add the test on Thursday?

Yeah sure I can add the test and then assign Josh to review!

@firestack firestack self-assigned this Jun 17, 2024
Copy link

Coverage of commit 37ccc6d

Summary coverage rate:
  lines......: 93.6% (3257 of 3480 lines)
  functions..: 73.4% (1343 of 1830 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack requested a review from joshlarson June 17, 2024 13:07
@firestack firestack enabled auto-merge (squash) June 17, 2024 14:51
@@ -1713,5 +1713,34 @@ describe("DiversionPage", () => {
screen.getByRole("combobox", { name: "Choose route" })
).toHaveAccessibleErrorMessage("Select a route to continue.")
})

test("while on this panel, route is not interactive", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome sauce!

This test is a perfect example of a test that will be a lot easier to write and work with once we build some better scaffolding for detour-related tests.

Thanks for adding it!

@firestack firestack merged commit 5e270be into main Jun 17, 2024
21 checks passed
@firestack firestack deleted the hp/suppress-interactive-map branch June 17, 2024 15:07
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