-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add validation errors to form fields for callouts #6722
Conversation
⚡️ Lighthouse report for the changes in this PRLighthouse tested 2 URLs Report for Article
Report for Front
|
Size Change: -19.5 kB (-1%) Total Size: 2.4 MB
ℹ️ View Unchanged
|
@@ -1,36 +1,43 @@ | |||
import { CampaignFieldSelect } from '../../../types/content'; | |||
import { Select as SourceSelect } from '@guardian/source-react-components'; | |||
import type { CampaignFieldSelect } from 'src/types/content'; |
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.
We're now using the select box from Source - happy days!
children={[ | ||
{ label: 'default', value: 'Please choose an option ...' }, | ||
] | ||
.concat(formField.options) |
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.
Source behaves in such a way that if a select box is mandatory, and the user has NOT changed the value, it will raise an error with the user. Therefore, we add a default 'Please choose an option' field to the start of any array of options passed down to us
Sorry - this is a mistake! Source does not provide us this behaviour. This logic will need to be added to form.tsx
. This will come in a subsequent PR. I've now removed this change.
<> | ||
<SourceTextArea | ||
data-testid={`form-field-${formField.id}`} | ||
label={formField.label} | ||
supporting={formField.description} |
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.
Previously, the description was not rendered on a text area. I think this is a mistake, so I've added it here.
Thanks for your helpful annotations in the PR comments! It's generally looking good to me, although I've noted a couple of issues. The |
<label | ||
css={[fieldLabelStyles, hideLabel && visuallyHidden]} | ||
htmlFor={formField.name} | ||
hidden={formField.hideLabel} |
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 hideLabel
property is now used directly on the label element. If this is true, we implement the visuallyHidden
style from Source. I believe that I am now essentially copying the same behaviour as Source (see here: https://github.com/guardian/csnx/blob/main/libs/%40guardian/source-react-components/src/label/Label.tsx)
If I am copying Source, that begs the question:
Q: Why have a custom FieldLabel
? Does Source not always provide us a label?
A: Yes - it does, but unfortunately not every single sort of form component we need is available in Source. Hence, we may need to occasionally create a custom form component and give it our bespoke FieldLabel
component for the label and description.
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.
nice write up, tom!
@@ -21,12 +23,14 @@ export const MultiSelect = ({ | |||
<div data-testid={`form-field-${formField.id}`}> | |||
{multiple ? ( | |||
<CheckboxSelect | |||
validationErrors={validationErrors ?? undefined} |
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.
Implemented @bryophyta suggested refactoring - thanks for the tip!
formField: CampaignFieldSelect; | ||
formData: { [key in string]: any }; | ||
setFormData: React.Dispatch<React.SetStateAction<{ [x: string]: any }>>; | ||
}; | ||
|
||
export const Select = ({ formField, formData, setFormData }: Props) => ( | ||
<> | ||
<FieldLabel formField={formField} /> |
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.
Now that we're using the Select
component from Source, there is no need to use a custom FieldLabel
- once again thanks for pointing that out @bryophyta
setFormData, | ||
}: Props) => ( | ||
<SourceSelect | ||
hideLabel={formField.hideLabel} |
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.
hideLabel
being used here as Source would like us to :)
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.
Really great work and nice explanation comments
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.
thanks for taking a look at the label issue, and thanks for such a clear write-up!
What does this change?
Callouts in DCR
This PR is part of a larger body of work to create a new Callout component in DCR. We are separating this PR into chunks so that it is easier to communicate the intent of each PR and to make it easier for DCR to review.
Why?
This PR adds more functionality to our callout form fields. We already have form components in the repo, but currently they do not take a custom validation error message which is necessary to display the custom styling provided by Source.
Previously, passing in custom validation information was not necessary as our legacy callout form did not use any form validation beyond what browsers provide by default. This meant that the only form of validation we could implement was to check if a required field was empty. This prevented us from using the custom Source error message styling as we need to pass a custom string down to the form field. With our
validationErrors
object, we can now do just this!How have I tested this?
I've tested it on DCR code and it works just as expected - there are no changes to the UI or behaviour of the current (soon to be legacy) form, which is just what we expect. This is going to become more useful in a subsequent PR that will perform custom form validation on a callout component.
Screenshots