-
Notifications
You must be signed in to change notification settings - Fork 9
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
Changes from 5 commits
23bd5eb
bb1ab5c
9f0b5bc
87f820a
a09633c
a9c43b6
d41364b
135faf1
cec1abd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -233,6 +233,11 @@ export const DiversionPage = ({ | |
onNavigateBack={editDetour} | ||
detourText={textArea} | ||
onChangeDetourText={setTextArea} | ||
onActivateDetour={ | ||
inTestGroup(TestGroups.DetoursList) | ||
? () => alert("Activate!") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I'm happy to remove it - I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
: undefined | ||
} | ||
/> | ||
) : null} | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import React from "react" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
For instance, all of the test cases that make assertions about which route-shape pieces are visible could be in 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() | ||
}) | ||
}) |
There was a problem hiding this comment.
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/buttonsThere was a problem hiding this comment.
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!)