LabeledField: Add a story for custom jsx for props #2426
Conversation
|
|
Size Change: 0 B Total Size: 98.8 kB ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-swtrokabok.chromatic.com/ Chromatic results:
|
beaesguerra
force-pushed
the
wb-1785-custom-example
branch
from
e034db4 to
6af4b4c
Compare
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 (e4eb849) 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="PR2426"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2426 |
| * Custom ReactNode elements can be used for the `label`, `description`, and | ||
| * `error` props. Ideally, the styling of LabeledField should not be overridden. | ||
| * If there is a specific use case where the styling needs to be overridden, | ||
| * please reach out to the Wonder Blocks team! |
There was a problem hiding this comment.
praise: This is great! thanks for calling out reaching out to us!
suggestion: How do you feel about including a message mentioning that this is useful for things like toggletips? or including icons that represent meaning? This is kinda tricky as I'm a firmly believer that we should use description (or inlining content) instead of relying on Tooltips. wdyt?
There was a problem hiding this comment.
I'm thinking we can leave it open-ended for now and see how often teams reach out about toggletips with fields. If it is a common pattern, I think we should create a separate prop for supporting the toggle tip so that it can be part of the API so it is consistent (ex: the same icon and styling/spacing is used for the hint).
Another thing to note is the contents of the label prop are wrapped with a <label> tag. I'm not sure if the toggle tip should be inside the label tag or outside, since clicking on the label will also focus on the associated field. And screen readers will read out the associated label when the field is interacted with, so the aria-label for the toggletip trigger will be included as part of that.
Inlining content in the description would simplify a lot of these things!
Another possible use case for the custom elements if adding the (optional) tag to the label. I think it isn't necessary to support this since we have the required indicator and we should stick with one of these approaches!
As always, let me know what you think!
🚀
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/labeled-field #2426 +/- ##
=============================================
=============================================
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:
The LabeledField accepts a ReactNode for the
label,description, anderrorMessageprops 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