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.

Sign up for GitHub

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(a11y): always enable continue button on FindProperty select page #3143

Closed
wants to merge 4 commits into from
Closed

fix(a11y): always enable continue button on FindProperty select page #3143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48a4be0
always enable continue button on findproperty map page
jessicamcinchak May 13, 2024
b2b0e03
wip
jessicamcinchak May 13, 2024
9a5ab94
fix default error msg
jessicamcinchak May 13, 2024
a6de4ca
rebase
jessicamcinchak May 14, 2024
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
42 changes: 27 additions & 15 deletions editor.planx.uk/src/@planx/components/FindProperty/Public/Autocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import find from "lodash/find";
import { parse, toNormalised } from "postcode";
import React, { useEffect, useState } from "react";
import InputLabel from "ui/public/InputLabel";
import ErrorWrapper from "ui/shared/ErrorWrapper";
import Input from "ui/shared/Input";

import type { SiteAddress } from "../model";
Expand All @@ -25,6 +26,12 @@ interface PickOSAddressProps {
initialSelectedAddress?: Option;
id?: string;
description?: string;
showPostcodeError: boolean;
setShowPostcodeError: React.Dispatch<React.SetStateAction<boolean>>;
addressAutocompleteError?: string;
setAddressAutocompleteError: React.Dispatch<
React.SetStateAction<string | undefined>
>;
}

const AutocompleteWrapper = styled(Box)(({ theme }) => ({
Expand Down Expand Up @@ -55,7 +62,6 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
const [selectedOption, setSelectedOption] = useState<Option | undefined>(
props.initialSelectedAddress ?? undefined,
);
const [showPostcodeError, setShowPostcodeError] = useState<boolean>(false);

// Fetch blpu_codes records so that we can join address CLASSIFICATION_CODE to planx variable
const { data: blpuCodes } = useQuery(FETCH_BLPU_CODES, {
Expand Down Expand Up @@ -109,6 +115,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
), // display value shown on PropertyInformation, should match <address-autocomplete /> options formatting
source: "os",
});
props.setAddressAutocompleteError(undefined);
}
};

Expand All @@ -124,7 +131,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
}, [sanitizedPostcode, selectedOption]);

const handleCheckPostcode = () => {
if (!sanitizedPostcode) setShowPostcodeError(true);
if (!sanitizedPostcode) props.setShowPostcodeError(true);
};

// XXX: If you press a key on the keyboard, you expect something to show up on the screen,
Expand All @@ -135,7 +142,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
if (selectedOption) {
// Reset the selected address on change of postcode to ensures no visual mismatch between address and postcode
setSelectedOption(undefined);
// Disable the "Continue" button if changing postcode before selecting new address after having come "back"
// Invalidate the "Continue" button if changing postcode before selecting new address after having come "back"
props.setAddress(undefined);
}

Expand All @@ -160,7 +167,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
id="postcode-input"
value={postcode || ""}
errorMessage={
showPostcodeError && !sanitizedPostcode
props.showPostcodeError && !sanitizedPostcode
? "Enter a valid UK postcode"
: ""
}
Expand All @@ -174,7 +181,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
maxLength: 8,
"aria-describedby": [
props.description ? DESCRIPTION_TEXT : "",
showPostcodeError && !sanitizedPostcode
props.showPostcodeError && !sanitizedPostcode
? `${ERROR_MESSAGE}-${props.id}`
: "",
]
Expand All @@ -184,16 +191,21 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
/>
</InputLabel>
{sanitizedPostcode && (
/* @ts-ignore */
<address-autocomplete
id="address-autocomplete"
data-testid="address-autocomplete-web-component"
postcode={sanitizedPostcode}
initialAddress={selectedOption?.title || ""}
osProxyEndpoint={`${process.env.REACT_APP_API_URL}/proxy/ordnance-survey`}
arrowStyle="light"
labelStyle="static"
/>
<ErrorWrapper
error={props.addressAutocompleteError}
id={"address-autocomplete"}
>
{/* @ts-ignore */}
<address-autocomplete
id="address-autocomplete"
data-testid="address-autocomplete-web-component"
postcode={sanitizedPostcode}
initialAddress={selectedOption?.title || ""}
osProxyEndpoint={`${process.env.REACT_APP_API_URL}/proxy/ordnance-survey`}
arrowStyle="light"
labelStyle="static"
/>
</ErrorWrapper>
Copy link
Member Author

@jessicamcinchak jessicamcinchak May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wrapping the web component in our error wrapper isn't perfect because it means the error message & select label will be out of normal order, with the error message above the label rather than below. I suspect the underlying markup may be a bit imperfect and disconnected too - but haven't inspected closely yet.

I see a couple possible ways forward:

  1. We choose not to address this "usability" feedback - on this page or this whole component?
  2. We always enable the continue button as suggested but ignore this style inconsistency for now if we confirm the markup is okay? It'll never be visible at the same time as the postcode error message, so hard to notice
  3. We update our web component code to implement an error wrapper directly there (and then create/pass a new error prop here?)
    • Looking through all the alpha gov examples of this component, none of them have error handling 🫠 so we'll have to make a number of decisions ourselves here about how to implement https://alphagov.github.io/accessible-autocomplete/examples/
    • This is probably ~1 more day of work. So feasible to still get across the line this week, but not trivial !
  4. Other suggestions?

Screenshot from 2024-05-13 20-36-07

Copy link
Member Author

Choose a reason for hiding this comment

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

Notes from dev call:

  • The right move will be to add error handling & the error wrapper directly in the web component
    • We'll want to additionally consider what this means for the autocomplete's current error state which is an icon & message like "No addresses in this postcode" (eg should it be a disabled select?)
  • As this has been flagged as usability only, and not an A-level issue, we are not going to try to implement this before our re-test next week, but rather before our next annual audit. Since there isn't a clear implementation example of this in the Gov UK alpha docs, we don't want to risk introducing an actual A-level issue at the re-test stage.
  • I'll close this PR and the Trello ticket will go to top of the icebox with these notes!

)}
</AutocompleteWrapper>
);
Expand Down
27 changes: 23 additions & 4 deletions editor.planx.uk/src/@planx/components/FindProperty/Public/Public.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,14 @@ describe("render states", () => {
expect(autocomplete.getAttribute("postcode")).toEqual("SE5 0HU");
expect(autocomplete.getAttribute("initialAddress")).toBeFalsy();

// expect continue to be disabled because an address has not been selected
expect(screen.getByTestId("continue-button")).toBeDisabled();
// user is unable to continue and to submit incomplete data
const continueButton = screen.getByTestId("continue-button");
expect(continueButton).toBeEnabled();
await user.click(continueButton);

expect(
screen.getByTestId("error-message-address-autocomplete"),
).toBeInTheDocument();
expect(handleSubmit).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -233,7 +239,11 @@ describe("render states", () => {
await screen.findByText("Type your postal code"),
).toBeInTheDocument();

expect(screen.getByTestId("continue-button")).toBeDisabled();
// user is unable to continue and submit incomplete data
expect(screen.getByTestId("continue-button")).toBeEnabled();
await user.click(screen.getByTestId("continue-button"));

expect(screen.getByText("Enter a valid UK postcode")).toBeInTheDocument();
expect(handleSubmit).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -316,11 +326,14 @@ describe("picking an OS address", () => {
});

it("updates the address-autocomplete props when the postcode is changed", async () => {
const handleSubmit = jest.fn();

const { user } = setup(
<MockedProvider mocks={findAddressReturnMock} addTypename={false}>
<FindProperty
description="Find your property"
title="Type your postal code"
handleSubmit={handleSubmit}
/>
</MockedProvider>,
);
Expand Down Expand Up @@ -351,7 +364,13 @@ describe("picking an OS address", () => {

// User is unable to continue and to submit incomplete data
const continueButton = screen.getByTestId("continue-button");
expect(continueButton).toBeDisabled();
expect(continueButton).toBeEnabled();
await user.click(continueButton);

expect(
screen.getByTestId("error-message-address-autocomplete"),
).toBeInTheDocument();
expect(handleSubmit).not.toHaveBeenCalled();
});

it("recovers previously submitted address when clicking the back button", async () => {
Expand Down
30 changes: 22 additions & 8 deletions editor.planx.uk/src/@planx/components/FindProperty/Public/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,13 @@ function Component(props: Props) {
: "os-address";
const [page, setPage] = useState<"os-address" | "new-address">(startPage);

const [showPostcodeError, setShowPostcodeError] = useState<boolean>(false);
const [addressAutocompleteError, setAddressAutocompleteError] =
useState<string>();
const [mapValidationError, setMapValidationError] = useState<string>();
const [showSiteDescriptionError, setShowSiteDescriptionError] =
useState<boolean>(false);
const [dataFetchError, setDataFetchError] = useState<string>();

const [address, setAddress] = useState<SiteAddress | undefined>(
previouslySubmittedData?._address,
Expand Down Expand Up @@ -111,7 +115,16 @@ function Component(props: Props) {
}, [data, address]);

const validateAndSubmit = () => {
// TODO `if (isValidating)` on either page, wrap Continue button in error mesage?
if (isValidating) {
setDataFetchError("Please wait for data fetching to complete");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

After the user has selected an address on either page, we need to make a quick request to Planning Data to fetch supplmental data about it that we can't get from Ordnance Survey - and we want to prevent the user from "continuing" before this fetch completes.

We used to simply set isValid={!isValidating} on the Card, but now we want to keep the continue button always enabled.

A couple questions here:

  • What's the best language for explaining this?
  • Is wrapping the "Continue" button in the error right considering the proposed design in feat: Inline loading indicator for address lookup #3136 ? If not the button, what else could we wrap?
    • [TODO] Prevent "Continue" button from expanding to full-screen width when in error state (see screenshot below)

Screenshot from 2024-05-13 21-02-50

Copy link
Member Author

Choose a reason for hiding this comment

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

Open questions from dev call:

  • Should we move the loading indicator into the button in these instances rather than wrap it in an error?


if (page === "os-address") {
if (address?.postcode === undefined) setShowPostcodeError(true);

if (address?.title === undefined)
setAddressAutocompleteError("Select an address");
}

if (page === "new-address") {
if (address?.x === undefined && address?.y === undefined)
Expand Down Expand Up @@ -155,11 +168,8 @@ function Component(props: Props) {
return (
<Card
handleSubmit={validateAndSubmit}
isValid={
page === "new-address" && !isValidating
? true
: Boolean(address) && !isValidating
}
isValid={true}
error={dataFetchError}
>
{getBody()}
</Card>
Expand Down Expand Up @@ -190,7 +200,7 @@ function Component(props: Props) {
/>
{Boolean(address) && isValidating && (
<DelayedLoadingIndicator
msDelayBeforeVisible={50}
msDelayBeforeVisible={0}
text="Fetching data..."
/>
)}
Expand All @@ -216,6 +226,10 @@ function Component(props: Props) {
}
id={props.id}
description={props.description || ""}
showPostcodeError={showPostcodeError}
setShowPostcodeError={setShowPostcodeError}
addressAutocompleteError={addressAutocompleteError}
setAddressAutocompleteError={setAddressAutocompleteError}
/>
{!props.allowNewAddresses ? (
<ExternalPlanningSiteDialog
Expand All @@ -238,7 +252,7 @@ function Component(props: Props) {
)}
{Boolean(address) && isValidating && (
<DelayedLoadingIndicator
msDelayBeforeVisible={50}
msDelayBeforeVisible={0}
text="Fetching data..."
/>
)}
Expand Down
27 changes: 15 additions & 12 deletions editor.planx.uk/src/@planx/components/shared/Preview/Card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import { useAnalyticsTracking } from "pages/FlowEditor/lib/analytics/provider";
import { useStore } from "pages/FlowEditor/lib/store";
import React, { useEffect } from "react";
import { ApplicationPath } from "types";
import ErrorWrapper from "ui/shared/ErrorWrapper";

import SaveResumeButton from "./SaveResumeButton";

interface Props {
children: React.ReactNode;
isValid?: boolean;
handleSubmit?: (data?: any) => void;
error?: string;
}

export const contentFlowSpacing = (theme: Theme): React.CSSProperties => ({
Expand Down Expand Up @@ -78,20 +80,21 @@ const Card: React.FC<Props> = ({
{...props}
>
{children}

<Box pt={2}>
{handleSubmit && (
<Button
variant="contained"
color="prompt"
size="large"
type="submit"
disabled={!isValid}
onClick={async () => await handleSubmit()}
data-testid="continue-button"
>
Continue
</Button>
<ErrorWrapper error={props.error} id="continue-button">
<Button
variant="contained"
color="prompt"
size="large"
type="submit"
disabled={!isValid}
onClick={async () => await handleSubmit()}
data-testid="continue-button"
>
Continue
</Button>
</ErrorWrapper>
)}
{showSaveResumeButton && <SaveResumeButton />}
</Box>
Expand Down
Loading