LabeledField: integrate with field components and fixes #2399
Conversation
…e so that external error messages set using the error prop can still be cleared. this is useful for clearing any backend errors. update tests for clearing error message on change
…assed down to the field prop
…change since the batch of labeled field work hasn't been released yet
…ent if it is set on either LabeledField or field component
…ere is an error message
…on and moving focus to the first field that has an error message
🦋 Changeset detectedLatest commit: 506cbd7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
|
Size Change: +89 B (+0.09%) Total Size: 98.8 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-dfcuccqawz.chromatic.com/ Chromatic results:
|
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@khanacademy/wonder-blocks-labeled-field": patch | |||
There was a problem hiding this comment.
Note: Although renaming a prop is technically a breaking change, I marked this as a patch change since the LabeledField component isn't released yet (it's in the feature/labeled-field branch so the previous prop hasn't been available for consumers. Let me know if there are any concerns with this!
There was a problem hiding this comment.
sounds good to me, thanks for the clarification!
| @@ -1,34 +0,0 @@ | |||
| import {Meta, Story, Canvas} from "@storybook/blocks"; | |||
There was a problem hiding this comment.
I combined this page with the Overview page so guidelines are together
| const field = screen.getByRole("textbox"); | ||
| await userEvent.type(field, "test"); | ||
| handleValidate.mockReset(); // Reset mock before leaving the field |
There was a problem hiding this comment.
Since we now clear the error state onChange when instantValidation=false (see use-field-validation.ts changes), onValidate is called each time a user types a character. We reset the mocks now before the action so we can more clearly check the onValidate calls triggered by the action
| * | ||
| * Note: Since the error icon has an aria-label, screen readers will | ||
| * prefix the error message with "Error:" (or the value provided to the | ||
| * errorIconAriaLabel in the `labels` prop) | ||
| */ | ||
| error?: React.ReactNode; | ||
| errorMessage?: React.ReactNode; |
There was a problem hiding this comment.
Renaming to errorMessage so it is clear it's the message. (Form components like TextField, TextArea, etc have error boolean props so it could be confusing if this one continuted to be named error when it doesn't expect a boolean)
| const isRequired = !!required || !!field.props.required; | ||
| const hasError = !!errorMessage || !!field.props.error; | ||
| const isLight = light || field.props.light; |
There was a problem hiding this comment.
We also check the field's props to see if LabeledField is required, is in an error state, or is in light mode.
This will make migration easier too so that we don't need to move light or required props from the field to the LabeledField component. The errorMessage prop would need to be set for LabeledField still since fields don't have this prop currently
There was a problem hiding this comment.
question: Would this work in the case where we use custom fields? I'm not sure if this will fail of a custom field doesn't contain one of these props.
There was a problem hiding this comment.
Good question!
If LabeledField and a custom field doesn't have the required/error/light props set, then these values will be false or undefined! (no issues here)
If a custom field doesn't support these props, then they won't be applied to the component. From testing out LabeledField with a plain <input> element, I get warnings around props not being supported or are the wrong type
Note: There is documentation around how LabeledField works best with field components that accept error and required props (I'll update this to include the light prop also) (?path=/docs/packages-labeledfield--docs#fields):
One idea is we could see if we can refine the field type so that the component must have these props supported.
What do you think?
There was a problem hiding this comment.
I added a story that shows LabeledField with a plain input element, let me know what you think!
There was a problem hiding this comment.
One idea is we could see if we can refine the field type so that the component must have these props supported.
I think it is fine not to enforce those props in field instances (at least not for now). I'm sure there will be cases where we still have reusable UI components in webapp that could be combined with LabeledField (e.g. DatePicker, TimePicker, some deprecated text field/area components). If we get to a point where we can get rid of those deprecated versions, then it would be nice to enforce those props.
Maybe these errors/warnings could go away using the nullish coalescing operator (??)
e.g.
const isRequired = !!required || !!field.props.required ?? false;
There was a problem hiding this comment.
Maybe these errors/warnings could go away using the nullish coalescing operator (??)
This doesn't address it unfortunately! The required prop isn't as much of an issue since it is a supported html attribute. It's more of the other props since those are specific to WB components! I was trying to see if there's a way for us to check at runtime if a component supports certain props before we set it. I couldn't find anything around this (let me know though if we have a way to do this!)
Some other options:
- LabeledField only sets html attributes. LabeledField works best though when the field components can handle their own required/error/light states and their testId.
- Use a renderField prop instead so that the props are passed as an argument and consumers could decide how to apply the props to their custom element
- We support both the current
fieldprop and arenderCustomFieldprop (see #2) - We leave things as is with the console warnings and custom components need to support the prop
Let me know what you think or if you have other ideas!
There was a problem hiding this comment.
Ahhh got it, thanks for clarifying. I'd say 4 would be best in this case as we still offer support to WB fields (making a good case for trying to use these components).
| */ | ||
| required?: boolean; | ||
| required?: boolean | string; |
There was a problem hiding this comment.
We update the required prop type to be the same as field components so it can be passed down
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (7c66299) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2399"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2399 |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@khanacademy/wonder-blocks-labeled-field": patch | |||
There was a problem hiding this comment.
sounds good to me, thanks for the clarification!
| TextField, TextArea, Choice, Checkbox, RadioButton, etc. | ||
|
|
||
|
|
||
| ## Best Practices |
There was a problem hiding this comment.
praise: Love this new consolidated section!
There was a problem hiding this comment.
Also shared these docs with the design team to see if they have any feedback or would like to add any other guidelines! The content now is based on our cubby meeting discussion a while back
| - Avoid disabling form submission buttons. There could be exceptions if the | ||
| button is for one field. | ||
| - If there are errors after a form is submitted, programatically move the user's | ||
| focus to the first field with an error |
There was a problem hiding this comment.
thought: I wonder if we should include static images at some point to illustrate how this could look like. No changes necessary, just food for thought.
There was a problem hiding this comment.
I included a Canvas of the LabeledField validation story so that there is an example of validation and focusing on the first field with an error after submission! ?path=/docs/packages-form-overview--docs
| @@ -6,7 +6,7 @@ import {TextArea} from "@khanacademy/wonder-blocks-form"; | |||
| import {spacing} from "@khanacademy/wonder-blocks-tokens"; | |||
|
|
|||
| export default { | |||
| title: "Packages / Form / Accessibility", | |||
| title: "", // Empty title so that these examples don't show in the sidebar. Examples are referenced in the .mdx file | |||
There was a problem hiding this comment.
praise: Neat! I didn't know about this.
There was a problem hiding this comment.
Disregard this! It wasn't working as expected, the story was still there in another section if I scrolled down 😅 I found if I named these stories the same as the Overview title, it isn't shown in the sidebar
| const isRequired = !!required || !!field.props.required; | ||
| const hasError = !!errorMessage || !!field.props.error; | ||
| const isLight = light || field.props.light; |
There was a problem hiding this comment.
question: Would this work in the case where we use custom fields? I'm not sure if this will fail of a custom field doesn't contain one of these props.
…o we can show an example of the focus moving to the first field
beaesguerra
force-pushed
the
wb-1783-labeled-field-with-updates
branch
from
786157e to
42fd023
Compare
| @@ -308,3 +564,18 @@ export const Scenarios = (args: PropsFor<typeof LabeledField>) => { | |||
| </View> | |||
| ); | |||
| }; | |||
|
|
|||
| /** | |||
| * Here is an example where LabeledField is used with a custom element. | |||
There was a problem hiding this comment.
suggestion: What do you think about adding some NOTE here for folks to be aware that this is available but that we still suggest using WB fields as much as they can?
There was a problem hiding this comment.
Added some more notes around using non wb components and how there'll be warnings if props aren't supported by the custom component
| const isRequired = !!required || !!field.props.required; | ||
| const hasError = !!errorMessage || !!field.props.error; | ||
| const isLight = light || field.props.light; |
There was a problem hiding this comment.
Ahhh got it, thanks for clarifying. I'd say 4 would be best in this case as we still offer support to WB fields (making a good case for trying to use these components).
| ]} | ||
| tag="label" | ||
| htmlFor={fieldId} | ||
| testId={testId && `${testId}-label`} | ||
| id={labelId} | ||
| > | ||
| {label} | ||
| {required && requiredIcon} | ||
| {isRequired && requiredIcon} | ||
| </LabelMedium> | ||
| <Strut size={spacing.xxxSmall_4} /> |
There was a problem hiding this comment.
thought: Looking at the stories, this got me thinking..... we should probably refactor this at some point to use gap instead of Strut (as we have been discussing previously). No need to make any changes here, but this would be a really nice improvement in the future (for all WB components).
There was a problem hiding this comment.
Good catch! I'll update this in a follow up PR :) Thanks!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/labeled-field #2399 +/- ##
=======================================================
Coverage ? 0
=======================================================
Files ? 0
Lines ? 0
Branches ? 0
=======================================================
Hits ? 0
Misses ? 0
Partials ? 0 Continue to review full report in Codecov by Sentry.
|
… `main` (#2429) ## Summary: This PR includes the following commits: - LabeledField (part 1): Initial set up for LabeledField component based on FieldHeading (#2322) - LabeledField(part2): refactor to a function component (#2338) - LabeledField(part3): Wire up attributes (#2339) - LabeledField(part4): styling and error icon (#2344) - LabeledField: integrate with field components and fixes (#2399) - Use LabeledField in other component stories (#2400) - Add more docs around different browser + screen reader behaviours for LabeledField (#2403) - LabeledField: Make sure custom required message is shown (#2425) - LabeledField: Add a story for custom jsx for props (#2426) - LabeledField: fix import (#2427) Issue: WB-1499 ## Test plan: - Confirm LabeledField is working as expected `?path=/docs/packages-labeledfield--docs` - Review docs for LabeledField: `?path=/docs/packages-labeledfield--docs` Author: beaesguerra Reviewers: beaesguerra, jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ gerald, ⌛ undefined Pull Request URL: #2429
Summary:
Putting LabeledField work together with field updates and addressing some issues. Fixes include:
TextField and TextArea:
aria-requiredbased onrequiredproperrorprop can still be cleared (This is useful for clearing any backend errors when the input value changes)LabeledField updates:
requiredprop be a boolean or string so it can be passed down to the field proprequired,errorandlightprops for LabeledField and field component if it is set on either LabeledField or field componenterrorprop toerrorMessage(This technically isn't a breaking change since the batch of labeled field work hasn't been released yet, labeled field work is currently in thefeature/labeled-fieldbranch)Stories/docs:
Issue: WB-1783
Test plan:
?path=/docs/packages-labeledfield--docs)/?path=/docs/packages-form-labeledtextfield-deprecated--docs)?path=/docs/packages-form-overview--docs)?path=/story/packages-labeledfield--required)?path=/story/packages-labeledfield--validation)Note: Did some preliminary testing with LabeledField and new validation features. Different screen reader and browser combinations had different behaviours around announcing errors so I created WB-1842 for more testing + documentation.