-
Notifications
You must be signed in to change notification settings - Fork 2
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 basic in-page feedback component #3841
Merged
Merged
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
55a5320
First pass at feedback component in storybook
jamdelion 012c6a4
fix: import
jamdelion f58d29a
Extract FaceBox component
jamdelion c9c23f0
Style a bit better
jamdelion 744aefc
Add rich text input
jamdelion b326811
Extract types and styled components
jamdelion ddbbb85
Add in typography
jamdelion 9903522
Merge branch 'main' into jh/feedback-component-refactoring
jamdelion b75a075
Fix some styling niggles
jamdelion 9458594
Fix hover padding
jamdelion c823e81
Fix hover issue
jamdelion d599915
Fix facebox text centering issue
jamdelion File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
19 changes: 19 additions & 0 deletions
19
editor.planx.uk/src/@planx/components/Feedback/Feedback.stories.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { Meta, StoryObj } from "@storybook/react"; | ||
|
||
import Public from "./Public"; | ||
|
||
const meta = { | ||
title: "PlanX Components/Feedback", | ||
component: Public, | ||
} satisfies Meta<typeof Public>; | ||
|
||
type Story = StoryObj<typeof meta>; | ||
|
||
export default meta; | ||
|
||
export const Basic = { | ||
args: { | ||
title: "Tell us what you think", | ||
privacyPolicyLink: "https://www.planx.uk/", | ||
}, | ||
} satisfies Story; |
32 changes: 32 additions & 0 deletions
32
editor.planx.uk/src/@planx/components/Feedback/Public.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { screen } from "@testing-library/react"; | ||
import React from "react"; | ||
import { setup } from "testUtils"; | ||
import { vi } from "vitest"; | ||
import { axe } from "vitest-axe"; | ||
|
||
import FeedbackComponent from "./Public"; | ||
|
||
const handleSubmit = vi.fn(); | ||
|
||
describe("when the Feedback component is rendered", async () => { | ||
it("should not have any accessibility violations", async () => { | ||
const { container } = setup( | ||
<FeedbackComponent title="Tell us what you think" />, | ||
); | ||
const results = await axe(container); | ||
expect(results).toHaveNoViolations(); | ||
}); | ||
it("should call handleSubmit when the continue button is pressed", async () => { | ||
const { user } = setup( | ||
<FeedbackComponent | ||
title="Tell us what you think" | ||
handleSubmit={handleSubmit} | ||
/>, | ||
); | ||
|
||
await user.click(screen.getByTestId("feedback-button-terrible")); | ||
await user.click(screen.getByTestId("continue-button")); | ||
|
||
expect(handleSubmit).toHaveBeenCalled(); | ||
}); | ||
}); |
119 changes: 119 additions & 0 deletions
119
editor.planx.uk/src/@planx/components/Feedback/Public.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
import Box from "@mui/material/Box"; | ||
import Grid from "@mui/material/Grid"; | ||
import Link from "@mui/material/Link"; | ||
import Typography from "@mui/material/Typography"; | ||
import Card from "@planx/components/shared/Preview/Card"; | ||
import { CardHeader } from "@planx/components/shared/Preview/CardHeader/CardHeader"; | ||
import type { PublicProps } from "@planx/components/ui"; | ||
import { useFormik } from "formik"; | ||
import React from "react"; | ||
import RichTextInput from "ui/editor/RichTextInput/RichTextInput"; | ||
import TerribleFace from "ui/images/feedback_filled-01.svg"; | ||
import PoorFace from "ui/images/feedback_filled-02.svg"; | ||
import NeutralFace from "ui/images/feedback_filled-03.svg"; | ||
import GoodFace from "ui/images/feedback_filled-04.svg"; | ||
import ExcellentFace from "ui/images/feedback_filled-05.svg"; | ||
import InputLabel from "ui/public/InputLabel"; | ||
|
||
import { getPreviouslySubmittedData, makeData } from "../shared/utils"; | ||
import { FaceBox } from "./components/FaceBox"; | ||
import { StyledToggleButtonGroup } from "./styled"; | ||
import { FeedbackComponentProps, FormProps } from "./types"; | ||
|
||
const FeedbackComponent = ( | ||
props: PublicProps<FeedbackComponentProps>, | ||
): FCReturn => { | ||
const formik = useFormik<FormProps>({ | ||
initialValues: getPreviouslySubmittedData(props) ?? { | ||
feedbackScore: "", | ||
feedback: "", | ||
}, | ||
onSubmit: (values) => { | ||
props.handleSubmit?.(makeData(props, values)); | ||
}, | ||
}); | ||
|
||
const handleFeedbackChange = ( | ||
event: React.MouseEvent<HTMLElement>, | ||
newValue: string | null, | ||
) => { | ||
if (newValue !== null) { | ||
formik.setFieldValue("feedbackScore", newValue); | ||
} | ||
}; | ||
|
||
return ( | ||
<Card handleSubmit={formik.handleSubmit}> | ||
<CardHeader title={props.title} /> | ||
<Typography pt={4}> | ||
This service is a work in progress, any feedback you share about your | ||
experience will help us to improve it. | ||
</Typography> | ||
<Typography pt={2}> | ||
Don't share any personal or financial information in your feedback. If | ||
you do we will act according to our{" "} | ||
<Link href={props.privacyPolicyLink ?? ""}>privacy policy</Link>. | ||
</Typography> | ||
|
||
<Box pt={2} mb={3}> | ||
<InputLabel label="How would you rate your experience with this service?" /> | ||
<StyledToggleButtonGroup | ||
value={formik.values.feedbackScore} | ||
exclusive | ||
id="feedbackButtonGroup" | ||
onChange={handleFeedbackChange} | ||
aria-label="feedback score" | ||
> | ||
<Grid container columnSpacing={15} component="fieldset"> | ||
<FaceBox | ||
value="1" | ||
testId="feedback-button-terrible" | ||
icon={TerribleFace} | ||
label="Terrible" | ||
altText="very unhappy face" | ||
/> | ||
<FaceBox | ||
value="2" | ||
icon={PoorFace} | ||
label="Poor" | ||
altText="slightly unhappy face" | ||
/> | ||
<FaceBox | ||
value="3" | ||
icon={NeutralFace} | ||
label="Neutral" | ||
altText="neutral face" | ||
/> | ||
<FaceBox | ||
value="4" | ||
icon={GoodFace} | ||
label="Good" | ||
altText="smiling face" | ||
/> | ||
<FaceBox | ||
value="5" | ||
icon={ExcellentFace} | ||
label="Excellent" | ||
altText="very happy face" | ||
/> | ||
</Grid> | ||
</StyledToggleButtonGroup> | ||
{/* </InputLabel> */} | ||
<InputLabel label="Please tell us more about your experience"> | ||
<RichTextInput | ||
name="feedback" | ||
value={formik.values.feedback} | ||
onChange={formik.handleChange} | ||
/> | ||
</InputLabel> | ||
</Box> | ||
<Typography variant="caption"> | ||
The information collected here isn't monitored by planning officers. | ||
Don't use it to give extra information about your project or submission. | ||
If you do, it cannot be used to assess your project. | ||
</Typography> | ||
</Card> | ||
); | ||
}; | ||
|
||
export default FeedbackComponent; |
48 changes: 48 additions & 0 deletions
48
editor.planx.uk/src/@planx/components/Feedback/components/FaceBox.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import Box from "@mui/material/Box"; | ||
import Grid from "@mui/material/Grid"; | ||
import ToggleButton from "@mui/material/ToggleButton"; | ||
import Typography from "@mui/material/Typography"; | ||
import React, { ReactElement } from "react"; | ||
|
||
interface FaceBoxProps { | ||
icon: string; | ||
label: string; | ||
altText: string; | ||
testId?: string; | ||
value: string; | ||
} | ||
|
||
export const FaceBox = ({ | ||
icon, | ||
label, | ||
altText, | ||
testId, | ||
value, | ||
}: FaceBoxProps): ReactElement => { | ||
return ( | ||
<Grid item xs={2} key={label}> | ||
<ToggleButton | ||
value={value} | ||
data-testid={testId} | ||
sx={{ | ||
px: 0, | ||
}} | ||
disableRipple | ||
> | ||
<Box | ||
sx={(theme) => ({ | ||
p: theme.spacing(2), | ||
border: `2px solid ${theme.palette.border.main} `, | ||
width: "120px", | ||
maxHeight: "120px", | ||
})} | ||
> | ||
<img src={icon} width={50} alt={altText} /> | ||
<Typography variant="body2" pt={0.5}> | ||
{label} | ||
</Typography> | ||
</Box> | ||
</ToggleButton> | ||
</Grid> | ||
); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { styled } from "@mui/material/styles"; | ||
import ToggleButtonGroup, { | ||
toggleButtonGroupClasses, | ||
} from "@mui/material/ToggleButtonGroup"; | ||
|
||
export const StyledToggleButtonGroup = styled(ToggleButtonGroup)( | ||
({ theme }) => ({ | ||
[`& .${toggleButtonGroupClasses.grouped}`]: { | ||
border: 0, | ||
padding: 0, | ||
marginTop: theme.spacing(1), | ||
}, | ||
paddingBottom: theme.spacing(3.5), | ||
}), | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { BaseNodeData } from "../shared"; | ||
|
||
export interface FeedbackComponentProps extends BaseNodeData { | ||
title: string; | ||
privacyPolicyLink?: string; | ||
fn?: string; | ||
} | ||
export interface FormProps { | ||
feedbackScore: string; | ||
feedback: string; | ||
} |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The hover issue was caused by having more than one child inside the label component, as discussed in this issue: mui/material-ui#14543
I've made the
children
prop optional in theInputLabel
component to get round this.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.
Ah nice one 👍
Alternatively, I think we could make children a
ReactElement
which would force a single element or fragment here. This will likely have cascading type issues but may point at other places where we're hitting a similar hover issue?The a11y tests are still passing in CI and Storybook without the wrapping label, so this is all good as-is.