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 Activate button gated behind detours-list test group #2713

Merged
merged 9 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions assets/css/_diversion_page.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@use "./color/definitions" as semantic;

.l-diversion-page {
display: grid;
// Default layout is linear
Expand Down Expand Up @@ -107,3 +109,7 @@
.c-diversion-panel__help-text {
line-height: 1.5;
}

.c-diversion-panel__activate-button {
background-color: semantic.$missed-stop;
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need bespoke css for this? I'd prefer to use a variant on the button itself https://react-bootstrap.github.io/docs/components/buttons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh - I didn't realize I could do that

(It gives it nicer hover behavior too!)

12 changes: 11 additions & 1 deletion assets/src/components/detours/detourFinishedPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ export const DetourFinishedPanel = ({
onNavigateBack,
detourText,
onChangeDetourText,
onActivateDetour,
}: {
onNavigateBack: () => void
detourText: string
onChangeDetourText: (value: string) => void
onActivateDetour?: () => void
}) => (
<Panel as="article">
<Panel.Header className="">
Expand Down Expand Up @@ -39,7 +41,7 @@ export const DetourFinishedPanel = ({
/>
</Panel.Body.ScrollArea>

<Panel.Body.Footer className="d-flex">
<Panel.Body.Footer className="d-flex flex-column">
<OverlayTrigger
placement="top"
trigger="click"
Expand All @@ -58,6 +60,14 @@ export const DetourFinishedPanel = ({
Copy Details
</Button>
</OverlayTrigger>
{onActivateDetour && (
<Button
className="m-3 flex-grow-1 c-diversion-panel__activate-button"
onClick={onActivateDetour}
>
Activate Detour
</Button>
)}
</Panel.Body.Footer>
</Panel.Body>
</Panel>
Expand Down
5 changes: 5 additions & 0 deletions assets/src/components/detours/diversionPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,11 @@ export const DiversionPage = ({
onNavigateBack={editDetour}
detourText={textArea}
onChangeDetourText={setTextArea}
onActivateDetour={
inTestGroup(TestGroups.DetoursList)
? () => alert("Activate!")
Copy link
Member

Choose a reason for hiding this comment

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

nit: I can't quite articulate why, so I guess take this as non-blocking, but I'd prefer to not use alert if we don't really need it.

Copy link
Collaborator

@hannahpurcell hannahpurcell Aug 1, 2024

Choose a reason for hiding this comment

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

Does knowing that this is only a temporary thing change your feeling? The alert is only in lieu of having hi-fi designs for this action

Copy link
Member

Choose a reason for hiding this comment

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

Slightly, I'm fine with it being non-blocking, but I'm inclined to enable the https://eslint.org/docs/latest/rules/no-alert rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I'm happy to remove it - I used alert because it was an easy way to locally test it, but I'm not attached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

: undefined
}
/>
) : null}
</div>
Expand Down
1 change: 1 addition & 0 deletions assets/src/userInTestGroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import getTestGroups from "./userTestGroups"
export enum TestGroups {
BackwardsDetourPrevention = "backwards-detour-prevention",
DemoMode = "demo-mode",
DetoursList = "detours-list",
DetoursPilot = "detours-pilot",
DummyDetourPage = "dummy-detour-page",
KeycloakSso = "keycloak-sso",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,9 @@ export default meta

type Story = StoryObj<typeof meta>

export const Story: Story = {}
export const WithActivateButton: Story = {}
export const WithoutActivateButton: Story = {
args: {
onActivateDetour: undefined,
},
}
86 changes: 86 additions & 0 deletions assets/tests/components/detours/diversionPageActivate.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import React from "react"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a thing I've been thinking about doing for a while.

diversionPage.test.tsx is getting very large, which makes it kind of unwieldy. There's no reason we need to have all of the detour tests live in the same file. I was thinking that we could start splitting out test cases for different angles of the detours feature into different test files.

For instance, all of the test cases that make assertions about which route-shape pieces are visible could be in diversionPageRouteShapes.test.tsx, and the test cases that assert when the undo and clear buttons are disabled or enabled might live in diversionPageUndoClearButtons.test.tsx.

Exactly how to divvy the use cases into files is a matter of taste, and I don't have strong feelings about it. But I figured that test cases related to saving/activating might be happiest away from everything else.

import {
DiversionPage as DiversionPageDefault,
DiversionPageProps,
} from "../../../src/components/detours/diversionPage"
import { originalRouteFactory } from "../../factories/originalRouteFactory"
import { beforeEach, describe, expect, jest, test } from "@jest/globals"
import "@testing-library/jest-dom/jest-globals"
import getTestGroups from "../../../src/userTestGroups"
import { TestGroups } from "../../../src/userInTestGroup"
import { act, fireEvent, render } from "@testing-library/react"
import userEvent from "@testing-library/user-event"
import {
activateDetourButton,
originalRouteShape,
reviewDetourButton,
} from "../../testHelpers/selectors/components/detours/diversionPage"
import {
fetchDetourDirections,
fetchFinishedDetour,
fetchNearestIntersection,
fetchRoutePatterns,
fetchUnfinishedDetour,
} from "../../../src/api"
import { neverPromise } from "../../testHelpers/mockHelpers"

beforeEach(() => {
jest.spyOn(global, "scrollTo").mockImplementationOnce(jest.fn())
})

const DiversionPage = (props: Partial<DiversionPageProps>) => {
return (
<DiversionPageDefault
originalRoute={originalRouteFactory.build()}
showConfirmCloseModal={false}
{...props}
/>
)
}

jest.mock("../../../src/api")
jest.mock("../../../src/userTestGroups")

beforeEach(() => {
jest.mocked(fetchDetourDirections).mockReturnValue(neverPromise())
jest.mocked(fetchUnfinishedDetour).mockReturnValue(neverPromise())
jest.mocked(fetchFinishedDetour).mockReturnValue(neverPromise())
jest.mocked(fetchNearestIntersection).mockReturnValue(neverPromise())
jest.mocked(fetchRoutePatterns).mockReturnValue(neverPromise())

jest
.mocked(getTestGroups)
.mockReturnValue([TestGroups.DetoursPilot, TestGroups.DetoursList])
})

describe("DiversionPage activate workflow", () => {
test("does not have an activate button on the review details screen if not in the detours-list test group", async () => {
jest.mocked(getTestGroups).mockReturnValue([TestGroups.DetoursPilot])

const { container } = render(<DiversionPage />)

act(() => {
fireEvent.click(originalRouteShape.get(container))
})
act(() => {
fireEvent.click(originalRouteShape.get(container))
})
await userEvent.click(reviewDetourButton.get())

expect(activateDetourButton.query()).not.toBeInTheDocument()
})

test("has an activate button on the review details screen", async () => {
const { container } = render(<DiversionPage />)

act(() => {
fireEvent.click(originalRouteShape.get(container))
})
act(() => {
fireEvent.click(originalRouteShape.get(container))
})
await userEvent.click(reviewDetourButton.get())

expect(activateDetourButton.get()).toBeVisible()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import { expect } from "@jest/globals"
import { byRole } from "testing-library-selector"

export const reviewDetourButton = byRole("button", { name: "Review Detour" })
export const activateDetourButton = byRole("button", {
name: "Activate Detour",
})

export const originalRouteShape = {
interactive: {
Expand Down
Loading