This is a feature-branch pull-request from feature/labeled-field to main
#2429
+2,546
−732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d on FieldHeading (#2322) ## Summary: - The `FieldHeading` component internal to the `wonder-blocks-form package` was copied to `wonder-blocks-labeled-field` package and renamed to the `LabeledField` component - Export `LabeledField` as a named export - Added base stories for `LabeledField` and updated docs Issue: WB-1503 ## Test plan: - `LabeledField` component is reviewed (`?path=/docs/packages-labeledfield-labeledfield--docs`) Note: Minimal changes have been made to the FieldHeading implementation and tests (just renaming). Next, I will be working on updating the component so it follows the implementation spec and exploring validation more, so the api may change! I'll be merging PRs to the `feature/labeled-field` branch until it's all ready 😄 Author: beaesguerra Reviewers: beaesguerra, jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Lint (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), 🚫 Chromatic - Get results on regular PRs, ✅ 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, ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Lint (ubuntu-latest, 20.x), 🚫 Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ gerald, ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ dependabot Pull Request URL: #2322
## Summary: Refactoring the LabeledField component to a function component. The LabeledField is based on the FieldHeading component and will be updated in more PRs to meet the implementation spec Issue: WB-1503 ## Test plan: Confirm tests still pass and component looks and functions the same way Author: beaesguerra Reviewers: jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Lint (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2338
## Summary: - Use an auto-generated unique id if the `id` prop is not provided - Wire up elements within the component - the `label` element's `for` attribute is the field `id` - `aria-describedby` on the field has the ids of the description and/or error message elements - Apply attributes to the field element: - `id` - `aria-describedby` - `required` if the `required` prop is set to `true` - `testId` Issue: WB-1503 ## Test plan: - Tests are passing - Updated documentation makes sense: - `?path=/docs/packages-labeledfield-labeledfield--docs` - `?path=/docs/packages-labeledfield-labeledfield-accessibility--docs` - There are no a11y warnings in the Storybook Accessibility add on for the LabeledField stories - Note: There are warnings for the LabeledField stories with the different field examples. They are related to an issue with the SearchField component and will be addressed in WB-1771! Author: beaesguerra Reviewers: beaesguerra, marcysutton, jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2339
## Summary: - Add error icon to the error message based on the [Figma specs](https://www.figma.com/design/VbVu3h2BpBhH80niq101MHHE/%F0%9F%92%A0-Main-Components?m=auto&node-id=13497-10015&t=BL8QsHXT6zAhJf3S-1) - This will address accessibility issues reported by Level Access related to "Ensure color is not the sole means of communicating information" for error messages ([example finding from the report](https://khanacademy.hub.essentia11y.com/short-link/uX7pyw_Y0AZ3Z7tz)) - Add a `labels` prop so that a translated label for the error icon can be passed in - Use semantic color tokens where possible to match what's in Figma - Handle long text with and without word breaks Issue: WB-1503 ## Test plan: - The styling in the LabeledField stories match the design specs (`?path=/docs/packages-labeledfield--docs`) - The error icon has a default aria-label (`?path=/story/packages-labeledfield--default`) <img width="1422" alt="Screenshot 2024-10-10 at 3 27 18 PM" src="https://github.com/user-attachments/assets/d04c6b41-3939-43e6-9115-f7cfbbbd9dd0"> - The error icon uses the labels prop for the aria-label (if provided) (`?path=/story/packages-labeledfield--default&args=labels.errorIconAriaLabel:placeholder+for+translated+aria+label`) <img width="1728" alt="Screenshot 2024-10-10 at 3 20 59 PM" src="https://github.com/user-attachments/assets/8d0f549d-d651-48ec-ab7c-faeea79c4f32"> - Overflowing text cases wrap properly (`?path=/story/packages-labeledfield--scenarios`) <img width="1411" alt="image" src="https://github.com/user-attachments/assets/e366a136-df99-480c-8f42-ce076c6d8adf"> - Confirm that when a user focuses on an invalid field, the error icon aria label and the error message are both read out (tested with VO + Safari, NVDA + Firefox, NVDA + Chrome) https://github.com/user-attachments/assets/230fe5ad-36f8-448b-9031-c46ef250ad97 Author: beaesguerra Reviewers: beaesguerra, marcysutton, jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2344
## Summary: Putting LabeledField work together with field updates and addressing some issues. Fixes include: TextField and TextArea: - Set `aria-required` based on `required` prop - Always clear error state onChange if instantValidation is false so that external error messages set using the `error` prop can still be cleared (This is useful for clearing any backend errors when the input value changes) LabeledField updates: - Let `required` prop be a boolean or string so it can be passed down to the field prop - Conditionally set spacing above the error message depending on if there is an error message - Set `required`, `error` and `light` props for LabeledField and field component if it is set on either LabeledField or field component - Rename `error` prop to `errorMessage` (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 the `feature/labeled-field` branch) Stories/docs: - Add docs on best practices for forms and fields - Add docs around LabeledTextField deprecation in favour of LabeledField + TextField - Update LabeledField examples and docs. Includes examples for validation and moving focus to the first field that has an error message after form submission Issue: WB-1783 ## Test plan: 1. Review new docs: - LabeledField (`?path=/docs/packages-labeledfield--docs`) - LabeledTextField (`/?path=/docs/packages-form-labeledtextfield-deprecated--docs`) - Form Best Practices (`?path=/docs/packages-form-overview--docs`) 2. Test LabeledField with Field Validation: Confirm validation works as expected - LabeledField Required story (`?path=/story/packages-labeledfield--required`) - Tabbing through fields without entering anything - Submitting without filling in fields - Inputting/selecting a value and removing it - LabeledField Validation story (`?path=/story/packages-labeledfield--validation`) - Triggering validation errors - Clearing validation errors when selected value changes - Submitting the form to see an example for backend errors 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. Author: beaesguerra Reviewers: beaesguerra, jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2399
## Summary: Use LabeledField in other component stories Issue: WB-1783 ## Test plan: Verify stories for the following components: - SingleSelect (`?path=/docs/packages-dropdown-singleselect--docs`) - MultiSelect (`?path=/docs/packages-dropdown-multiselect--docs`) - TextArea (`?path=/docs/packages-form-textarea--docs`) - TextArea Variants (`?path=/docs/packages-form-textarea-all-variants--docs`) - TextField (`?path=/docs/packages-form-textfield--docs`) - TextField Variants (`?path=/docs/packages-form-textfield-all-variants--docs`) - SearchField (`?path=/docs/packages-searchfield--docs`) - SearchField Variants (`?path=/docs/packages-searchfield-all-variants--docs`) Note: This addresses Storybook a11y issues related to form fields and labels. There are some stories that focus on the form field without the label so those warnings will need to be handled separately! Author: beaesguerra Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ dependabot Pull Request URL: #2400
… LabeledField (#2403) ## Summary: Documenting the different browser + screen reader behaviours for reference since there are some browser/screen reader inconsistencies! Issue: WB-1842 ## Test plan: - Review new LabeledField Accessibility docs (`?path=/docs/packages-labeledfield-accessibility--docs`) - Review the LabeledField stories with different screen readers/browsers, especially around errors (would appreciate any feedback around this!) ## Recordings: ### Interacting with fields with errors Safari + VoiceOver https://github.com/user-attachments/assets/4913f5e6-6af5-42fb-996c-c2bed380b37a Chrome + NVDA https://github.com/user-attachments/assets/230aa0ec-07b7-47e5-bb3e-98e1e65a5908 Firefox + NVDA https://github.com/user-attachments/assets/1a9a4b9d-7c97-455b-a830-63f9e90a852d ### Error validation while filling out fields Safari + VoiceOver https://github.com/user-attachments/assets/37cbf220-52b8-4078-8872-d431379659d5 Chrome + NVDA https://github.com/user-attachments/assets/2771b228-665d-48f5-937c-664fd0f11326 Firefox + NVDA https://github.com/user-attachments/assets/fbd5d458-0567-4095-9ed0-73e2d6a836c5 ### Error validation after submission Safari + VoiceOver https://github.com/user-attachments/assets/4166caae-0769-488d-af8a-444ad1381e1d Chrome + NVDA https://github.com/user-attachments/assets/8bb07bb8-6669-4e88-b968-2484d75c6c0f Firefox + NVDA https://github.com/user-attachments/assets/1eb10a99-5c87-406a-9e15-994f603b2fc6 Author: beaesguerra Reviewers: beaesguerra, jandrade, marcysutton Required Reviewers: Approved By: jandrade, marcysutton Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Publish npm snapshot (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), ✅ gerald, ⏭️ dependabot Pull Request URL: #2403
## Summary: Makes sure custom required messages passed into `LabeledField` or the `field` prop are displayed Issue: WB-1848 ## Test plan: Confirm in `?path=/story/packages-labeledfield--required` that the error message shows the custom required message Author: beaesguerra Reviewers: beaesguerra, jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ 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), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ dependabot, ✅ gerald Pull Request URL: #2425
## Summary: The LabeledField accepts a ReactNode for the `label`, `description`, and `errorMessage` props already. This adds a story and documentation for this functionality! Issue: WB-1785 ## Test plan: Review Custom story docs in `?path=/docs/packages-labeledfield--docs#custom` Author: beaesguerra Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: jandrade Checks: ✅ 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 (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), ✅ 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), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2426
## Summary: - Fix import to ComponentInfo for feature branch now that it is up to date with main. This fixes the lint and type errors in the feature branch - For context, ComponentInfo was moved to a new location after the initial set up of the LabeledField work Issue: WB-XXXX ## Test plan: - Checks pass on the PR Author: beaesguerra Reviewers: jandrade Required Reviewers: Approved By: jandrade Checks: ✅ 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 (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), ✅ 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), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ dependabot, ✅ gerald Pull Request URL: #2427
🦋 Changeset detectedLatest commit: a37305a 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: +1.86 kB (+1.93%) Total Size: 98.3 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-kxfitkzptu.chromatic.com/ Chromatic results:
|
beaesguerra
commented
Jan 15, 2025
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@khanacademy/wonder-blocks-labeled-field": patch | |||
There was a problem hiding this comment.
Note: all of the changesets are marked either a patch or minor update for the wonder-blocks-labeled-field package.
Currently, the package is at 1.0.5 and exports a string as default as a placeholder (ref).
I was wondering if it would be worth changing to a major version. It isn't currently used in webapp or perseus though, so it might be fine if we leave it at v1.x.x. The new version will be 1.1.0
Let me know if anyone has any thoughts or concerns around this!
There was a problem hiding this comment.
IMO It's fine marking it as non-breaking change as there's no current usage
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 (0ca2f7d) 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="PR2429"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2429 |
jandrade
approved these changes
Jan 15, 2025
Comment on lines
+39
to
+40
| * - If the `light` prop is set on either `LabeledField` or the `field` prop, | ||
| * both components will render in light mode |
There was a problem hiding this comment.
question: How do you feel about removing all light mentions here? I'd say it'd be better tackling it in a separate PR (after landing this branch), but wanted to get your thoughts on this. Same question with the Light story.
There was a problem hiding this comment.
Agreed - we can remove the mentions of the light prop in the docs in a follow up PR!
|
Testing with the snapshot in webapp looks good, will just need to update some jest snapshots to account for adding the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR includes the following commits:
Issue: WB-1499
Test plan:
?path=/docs/packages-labeledfield--docs?path=/docs/packages-labeledfield--docs