Feat: add required indicators to form fields #2815
Conversation
…ents - Introduced a `required` prop to the Label component to indicate mandatory fields. - Updated Input, Textarea, NumberInput, Select, Checkbox, RadioGroup, and other form components to utilize the new `required` prop for better accessibility and user experience.
🦋 Changeset detectedLatest commit: 527be2d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
matthewvolk
left a comment
matthewvolk
left a comment
There was a problem hiding this comment.
Could you generate a changeset for this? You can run pnpm changeset from the project root and walk through the prompts. Just include a 1-2 sentence description of the feature, and copy/paste the migration guide from your PR description and the changeset should be good to commit and add to this PR 👍
|
Added translations and a changeset cc @jorgemoya @matthewvolk |
jorgemoya
left a comment
jorgemoya
left a comment
There was a problem hiding this comment.
Marking as request changes only because there's an oversight that will break form components until fixed.
core/messages/da.json
Outdated
| "Form": { | ||
| "optional": "valgfri" |
There was a problem hiding this comment.
These translated labels get auto generated every Monday, so for future references as long as we have the en label that is fine. next-intl will fallback to the en label if there is no available translation.
There was a problem hiding this comment.
Updated to only keep the english version
|
Removed all translations except for english and made sure to pass |
| id={id !== undefined ? `${id}-label` : `${generatedId}-label`} | ||
| > | ||
| {label} | ||
| {!props.required && <span className="ml-1 normal-case">(optional)</span>} |
There was a problem hiding this comment.
Still not using the translated labels
There was a problem hiding this comment.
Removed this text from checkbox since inherently it doesn't make sense. Checkboxes are inherently optinoal
There was a problem hiding this comment.
Checkbox fields can still be required by the Bigcommerce GQL api.
Thinking of general cases, a ToS checkbox would be required.
There was a problem hiding this comment.
There's a difference though between requiring a value and requiring that the box be checked right? Technically, an unchecked box is still a valid user input? Does that make sense?
There was a problem hiding this comment.
Isn't an unckecked checkbox looked at as a boolean false?
There was a problem hiding this comment.
@jorgemoya updated to pass field.required to CheckboxGroup which handles displaying the label using the Label component for a group of individual Checkbox components. This matches the Stencil functionality. The difference is that we display optional text instead of required text.
| } | ||
| }, | ||
| "Form": { | ||
| "optional": "optional" |
There was a problem hiding this comment.
🍹Should punctuation e.g., (, ) be included in next-intl strings? Wondering if other languages use a different convention.
There was a problem hiding this comment.
I wondered about that. Not that I know of. I also wouldn't think the automation we have set up would have an automatic/appropriate translation if that was the case. I wouldn't think our automatic translations would understand the use case just based on the text enough to make a decision?
What/Why?
Added
(optional)text to optional fields in forms so that it's clear which ones are required vs notTesting
Open the
/registerroute and see a combination of required and optional fields.Migration
The new
requiredprops are optional, so they are backwards compatible. However, this does mean that the(optional)text will now show up on fields that aren't explicitly marked as required by passing therequiredprop to theLabelcomponent.CATALYST-1672