-
Notifications
You must be signed in to change notification settings - Fork 30
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(Form + GridForm): Add actual required optional + required form text #2872
Conversation
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.
It works!! ✨
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.
LGTM! Left some comments and questions :)
Tested the voiceover on required and optional text and they're not read :D
{showRequired ? ' *' : ''} | ||
{!isSoloField && ( | ||
<Text as="span" aria-hidden> | ||
{required ? '*' : ' \u00A0(optional)'} |
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.
' \u00A0
is the space before the \u00A0
intentional?
When looking up \u00A0
, I see it's a non-break space, so to me, this reads as two spaces. But then, it looks like the tests pass, and the stories look fine 🤔
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.
i was having issues when not including the hardcoded space but I'll double check!
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.
definitely one of those things where I'm more curious than anything lol
|
||
export function GridForm<Values extends FormValues<Values>>({ | ||
cancel, | ||
children, | ||
columnGap = defaultColumnGap, | ||
fields = [], | ||
hasSoloField = false, | ||
hideRequiredText, |
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.
can this be defaulted to false
?
@@ -24,6 +27,8 @@ export const GridFormContent: React.FC<GridFormContentProps> = ({ | |||
field.validation?.required | |||
); | |||
|
|||
// console.log(isSoloField); |
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.
leftover comment
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.
ty!! will fix!
@@ -34,6 +34,8 @@ The primary access pattern of `ConnectedForm` is the `useConnectedForm` hook whi | |||
|
|||
Each of your fields' names must correspond with the appropriate `defaultValue` key. `validationRules` operates similarly - each key must correspond to a key in `defaultValue` and must follow [react-hook-form's](https://react-hook-form.com) [validation patterns](https://react-hook-form.com/v6/api#register). | |||
|
|||
This hook also returns the `FormRequiredText` component - include this before your form unless all of your form fields are optional. |
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.
Is there a form where all fields are optional?
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.
yupp! a good example is the job-readiness page -- all of the fields are optional (you do need one of them to submit the form but it can be any one of them)
|
||
## hideRequiredText | ||
|
||
`hideRequiredText` will hide the ;\* Required' text that appears at the top of our forms. This should only be hidden if the form has no required fields. |
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.
;* Required'
typo, the semicolon should be a single quote
📬Published Alpha Packages:[email protected] |
🚀 Styleguide deploy preview ready! |
Overview
Adds * Required to GridForm (and as a FormElement) + (optional) to labels of optional field. Also removed
showRequired
from GridForm because the accessibility text is mandatoryTheres also some reorganizing of form elements into folders
PR Checklist
Testing Instructions
PR Links and Envs