Skip to content

Commit

Permalink
web: implement UIButton.RequiresConfirmation (#5218)
Browse files Browse the repository at this point in the history
  • Loading branch information
landism committed Nov 25, 2021
1 parent b6b49d4 commit cd387bc
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 21 deletions.
61 changes: 55 additions & 6 deletions web/src/ApiButton.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
cleanupMockAnalyticsCalls,
expectIncrs,
mockAnalyticsCalls,
nonAnalyticsCalls,
} from "./analytics_test_helpers"
import {
ApiButton,
Expand All @@ -24,6 +25,7 @@ import {
textField,
} from "./ApiButton.testhelpers"
import { HudErrorContextProvider } from "./HudErrorContext"
import { InstrumentedButton } from "./instrumentedComponents"
import { accessorsForTesting, tiltfileKeyContext } from "./LocalStorage"
import { flushPromises } from "./promise"

Expand Down Expand Up @@ -180,9 +182,7 @@ describe("ApiButton", () => {
await click(submit)
root.update()

const calls = fetchMock
.calls()
.filter((c) => c[0] !== "http://localhost/api/analytics")
const calls = nonAnalyticsCalls()
expect(calls.length).toEqual(1)
const call = calls[0]
expect(call[0]).toEqual(
Expand Down Expand Up @@ -233,9 +233,7 @@ describe("ApiButton", () => {
await click(submit)
root.update()

const calls = fetchMock
.calls()
.filter((c) => c[0] !== "http://localhost/api/analytics")
const calls = nonAnalyticsCalls()
expect(calls.length).toEqual(1)
const call = calls[0]
expect(call[0]).toEqual(
Expand Down Expand Up @@ -335,6 +333,57 @@ describe("ApiButton", () => {

expect(error).toEqual("Error submitting button click: broken!")
})

it("when requiresConfirmation is set, a second confirmation click to submit", async () => {
const b = makeUIButton({ requiresConfirmation: true })
const root = mountButton(b)

let button = root.find(InstrumentedButton)
expect(button.find(ApiButtonLabel).text()).toEqual(b.spec?.text)

await click(button.find(Button).at(0))
root.update()

// after first click, should show a "Confirm", and not have submitted the click to the backend
button = root.find(InstrumentedButton)
expect(button.find(ApiButtonLabel).text()).toEqual("Confirm")
expect(nonAnalyticsCalls().length).toEqual(0)

await click(button.find(Button).at(0))
root.update()

// after second click, button text reverts and click is submitted
button = root.find(InstrumentedButton)
expect(button.find(ApiButtonLabel).text()).toEqual(b.spec?.text)
expect(nonAnalyticsCalls().length).toEqual(1)
})

it("when requiresConfirmation is set, allows canceling instead of confirming", async () => {
const b = makeUIButton({ requiresConfirmation: true })
const root = mountButton(b)

let button = root.find(InstrumentedButton)
expect(button.find(ApiButtonLabel).text()).toEqual(b.spec?.text)

await click(button.find(Button).at(0))
root.update()

button = root.find(InstrumentedButton)
expect(button.find(ApiButtonLabel).text()).toEqual("Confirm")
expect(nonAnalyticsCalls().length).toEqual(0)

// second button cancels confirmation
const cancelButton = button.find(Button).at(1)
expect(cancelButton.props()["aria-label"]).toEqual(`Cancel ${b.spec?.text}`)

await click(cancelButton)
root.update()

// upon clicking, the button click is aborted and no update is sent to the server
button = root.find(InstrumentedButton)
expect(button.find(ApiButtonLabel).text()).toEqual(b.spec?.text)
expect(nonAnalyticsCalls().length).toEqual(0)
})
})

async function click(button: any) {
Expand Down
2 changes: 2 additions & 0 deletions web/src/ApiButton.testhelpers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export function hiddenField(name: string, value: string): UIInputSpec {
export function makeUIButton(args?: {
inputSpecs?: UIInputSpec[]
inputStatuses?: UIInputStatus[]
requiresConfirmation?: boolean
}): UIButton {
return {
metadata: {
Expand All @@ -52,6 +53,7 @@ export function makeUIButton(args?: {
componentType: "Global",
componentID: "nav",
},
requiresConfirmation: args?.requiresConfirmation,
},
status: {
inputs: args?.inputStatuses,
Expand Down
73 changes: 62 additions & 11 deletions web/src/ApiButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Link } from "react-router-dom"
import styled from "styled-components"
import { Tags } from "./analytics"
import { annotations } from "./annotations"
import { ReactComponent as CloseSvg } from "./assets/svg/close.svg"
import FloatDialog from "./FloatDialog"
import { useHudErrorContext } from "./HudErrorContext"
import {
Expand Down Expand Up @@ -57,6 +58,25 @@ export const LogLink = styled(Link)`
font-size: ${FontSize.smallest};
padding-left: ${SizeUnit(0.5)};
`
const ConfirmButton = styled(InstrumentedButton)`
&& {
background-color: ${Color.red};
border-color: ${Color.gray};
color: ${Color.black};
&:hover,
&:active,
&:focus {
background-color: ${Color.red};
border-color: ${Color.redLight};
color: ${Color.black};
}
}
.fillStd {
fill: ${Color.black} !important; /* there's some style somewhere with a higher level of specificity than this one that i did not spend time trying to find... */
}
`

type ApiButtonProps = ButtonProps & {
className?: string
Expand Down Expand Up @@ -287,17 +307,6 @@ async function updateButtonStatus(
await tiltApiPut("uibuttons", "status", toUpdate)
}

function setHiddenInputs(
uiButton: UIButton,
inputValues: { [name: string]: any }
) {
uiButton.spec?.inputs?.forEach((i) => {
if (i.hidden && i.name) {
inputValues[i.name] = i.hidden.value
}
})
}

// Renders a UIButton.
// NB: The `Button` in `ApiButton` refers to a UIButton, not an html <button>.
// This can be confusing because each ApiButton consists of one or two <button>s:
Expand All @@ -312,6 +321,8 @@ export function ApiButton(props: React.PropsWithChildren<ApiButtonProps>) {
[name: string]: any
}>(`apibutton-${uiButton.metadata?.name}`, {})

const [confirming, setConfirming] = useState(false)

const { enqueueSnackbar } = useSnackbar()
const pb = usePathBuilder()

Expand All @@ -332,6 +343,15 @@ export function ApiButton(props: React.PropsWithChildren<ApiButtonProps>) {
}

const onClick = async () => {
if (uiButton.spec?.requiresConfirmation && !confirming) {
setConfirming(true)
return
}

if (confirming) {
setConfirming(false)
}

// TODO(milas): currently the loading state just disables the button for the duration of
// the AJAX request to avoid duplicate clicks - there is no progress tracking at the
// moment, so there's no fancy spinner animation or propagation of result of action(s)
Expand Down Expand Up @@ -368,6 +388,37 @@ export function ApiButton(props: React.PropsWithChildren<ApiButtonProps>) {

const disabled = loading || uiButton.spec?.disabled

if (confirming) {
return (
<ApiButtonRoot
className={className}
disableRipple={true}
aria-label={uiButton.spec?.text}
disabled={disabled}
>
<ConfirmButton
analyticsName={"ui.web.uibutton"}
analyticsTags={{ confirm: "true" }}
onClick={onClick}
disabled={disabled}
aria-label={`Confirm ${uiButton.spec?.text}`}
{...buttonProps}
>
<ApiButtonLabel>Confirm</ApiButtonLabel>
</ConfirmButton>
<ConfirmButton
analyticsName={"ui.web.uibutton"}
analyticsTags={{ unconfirm: "true" }}
onClick={() => setConfirming(false)}
aria-label={`Cancel ${uiButton.spec?.text}`}
{...buttonProps}
>
<CloseSvg />
</ConfirmButton>
</ApiButtonRoot>
)
}

// button text is not included in analytics name since that can be user data
const button = (
<InstrumentedButton
Expand Down
10 changes: 6 additions & 4 deletions web/src/OverviewActionBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,13 @@ const DisableButton = styled(ApiButton)`
}
}
.MuiButton-label {
// hardcode a width to workaround this bug:
// https://app.shortcut.com/windmill/story/12912/uibuttons-created-by-togglebuttons-have-different-sizes-when-toggled
width: ${SizeUnit(3.6)};
button:first-child {
width: 100%;
}
// hardcode a width to workaround this bug:
// https://app.shortcut.com/windmill/story/12912/uibuttons-created-by-togglebuttons-have-different-sizes-when-toggled
width: ${SizeUnit(4.4)};
`

const ButtonRoot = styled(InstrumentedButton)`
Expand Down
6 changes: 6 additions & 0 deletions web/src/analytics_test_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,9 @@ export function expectIncrs(...incrs: { name: string; tags: Tags }[]) {
)
expect(actualRequestBodies).toEqual(expectedRequestBodies)
}

export function nonAnalyticsCalls() {
return fetchMock
.calls()
.filter((c) => c[0] !== "http://localhost/api/analytics")
}

0 comments on commit cd387bc

Please sign in to comment.