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.

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 explanation of asterisk for required fields to example forms and documentation #820

Merged
merged 13 commits into from
Nov 22, 2024
Merged
40 changes: 12 additions & 28 deletions apps/docs/content/components/FormControl.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ description: Use the form control component to display form inputs alongside lab
import InlineCode from '@primer/gatsby-theme-doctocat/src/components/inline-code'
import {PropTableValues} from '../../src/components'
import {Label} from '@primer/react'
import {Heading} from '@primer/react-brand'
import {Heading, Text} from '@primer/react-brand'
import {SearchIcon} from '@primer/octicons-react'
import {Link} from 'gatsby'

Expand Down Expand Up @@ -83,6 +83,9 @@ An example of a typical layout composed using `FormControl`:
paddingBottom: 3,
}}
>
<Text as="p" variant="muted" size="100">
All fields marked with an asterisk (*) are required
</Text>
<Container
sx={{
display: 'grid',
Expand Down Expand Up @@ -229,45 +232,24 @@ Try changing the input value to to `monalisa` to show the `success` state.

```javascript live noinline
const App = () => {
const [value, setValue] = React.useState()
const [validationState, setValidationState] = React.useState()

React.useEffect(() => {
const defaultHandle = 'mona lisa'
setValue(defaultHandle)
validate(defaultHandle)
}, [])

const validate = (inputValue) => {
if (/\s/g.test(inputValue)) {
setValidationState('error')
} else {
setValidationState('success')
}
}
const [value, setValue] = React.useState('mona lisa')

const handleChange = (event) => {
event.preventDefault()
if (!event.target.value) {
setValue(undefined)
setValidationState(undefined)
return
}
setValue(event.target.value)
validate(event.target.value)
}

const isInvalid = /\s/g.test(value)
rezrah marked this conversation as resolved.
Show resolved Hide resolved

return (
<FormControl validationStatus={validationState}>
<FormControl validationStatus={isInvalid ? 'error' : 'success'}>
<FormControl.Label>GitHub handle</FormControl.Label>
<TextInput onChange={handleChange} value={value} fullWidth />
{validationState && validationState === 'error' && (
{isInvalid ? (
<FormControl.Validation>
GitHub handles cannot contain spaces.{' '}
{value && `Did you mean "${value.replaceAll(' ', '')}"`}
</FormControl.Validation>
)}
{validationState && validationState === 'success' && (
) : (
<FormControl.Validation>Valid name</FormControl.Validation>
)}
</FormControl>
Expand Down Expand Up @@ -328,6 +310,8 @@ Pass the `fullWidth` prop to `FormControl` to provide additional behavior and st

Pass the `required` prop to `FormControl` to provide additional behavior and state context to all `children`, rather than the input only.

When marking a field as required, it is recommended to also provide a corresponding message at the start of the form informing the user that "_all fields marked with an asterisk (\*) are required_".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Could we also update the form example further up the page please?

Screenshot 2024-11-15 at 16 26 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good spot, thanks.

I'd searched for all <form> tags but looks like that was missing one. I've wrapped it in one now and made the required change 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @joshfarrant.

@simmonsjenna here's a sample of the required field statement we need Site to add to all form designs going forward. Let us know if you have any further guidance on spacing, positioning or colors. We can fix in a follow up.

Screenshot 2024-11-21 at 10 45 15

Link

Choose a reason for hiding this comment

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

@rezrah LGTM


```jsx live
<FormControl required>
<FormControl.Label>Name</FormControl.Label>
Expand Down
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea what's going on with these? Presumably it's the avatar being switched to a local fixture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I'm not sure, that's all I can imagine but I don't understand why that would trigger an update 🤔

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.
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
5 changes: 5 additions & 0 deletions packages/react/src/forms/TextInput/TextInput.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,14 @@
}

.TextInput-wrapper--fullWidth {
width: 100%;
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
display: flex;
}

.TextInput-wrapper:not(.TextInput-wrapper--fullWidth) {
width: min-content;
}

.TextInput-wrapper--monospace {
font-family: var(--brand-fontStack-monospace);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/react/src/forms/form.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ export const GitHubEnterprise = args => {
marginBottom: 32,
}}
>
<Text as="p" variant="muted" size="100">
All fields marked with an asterisk (*) are required
</Text>
<div
style={{
display: 'flex',
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading