docs(fieldlabel/form): migrates docs to storybook by marissahuysentruyt · Pull Request #3165 · adobe/spectrum-css

marissahuysentruyt · 2024-09-25T15:57:50Z

Description

This continues the effort to migrate documentation from the CSS static docs site into Storybook, with a focus on fieldlabel and form.

In each component's case, missing stories and corresponding documentation were added, along with any relevant S2 features from feat(fieldlabel)!: s2 migration #2569.
Test coverage was expanded for form, and "cleaned up" for fieldlabel.
form also now has a fieldLabelAlignment control, to better showcase left- or right-aligned field labels.

This PR has no CSS changes, so no changeset is needed.

Jira/Specs

CSS-932

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Pull down the branch to run locally or use the deploy preview
Verify all stories and documentation from the static docs site are represented on the Storybook field label docs page
Verify the Chromatic testing grid for field label also captures all variants
Verify all stories and documentation from the static docs site are represented on the Storybook form docs page
Verify the Chromatic testing grid for form covers all new variants
The newly added documentation has been proofread and makes sense.

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-09-25T15:57:54Z

⚠️ No Changeset found

Latest commit: 6c3319c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-09-25T16:01:26Z

🚀 Deployed on https://pr-3165--spectrum-css.netlify.app

github-actions · 2024-09-25T16:02:45Z

File metrics

Summary

Total size: 4.31 MB^*

🎉 No changes detected in any packages

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

marissahuysentruyt · 2024-09-25T18:14:46Z

-		labelsAbove: {
-			name: "Labels above",
-			type: { name: "boolean" },
+		labelPosition: {


I changed this to labelPosition because both text field and slider have similar controls called labelPosition.

marissahuysentruyt · 2024-09-25T18:15:47Z

 };

+/**
+ * A stacked layout with the labels above the form fields. The class `.spectrum-Form--labelPosition` is applied to the form itself. You do not need to apply a specific class to the field label because `.spectrum-FieldLabel--left` is applied to each to each [field label](/docs/components-field-label--docs) by default.


According to the field label docs, top labels are default, which is why the default story is changed. https://spectrum.adobe.com/page/field-label/#Label-position

When looking at textfield, if I toggle "displayFieldLabel," that also defaults to a top label.

marissahuysentruyt · 2024-09-25T18:19:54Z

+RightAligned.storyName = "Right-aligned";
+
+/**
+ * Field labels for required inputs can be marked with an asterisk at the end of the label. Optional inputs would then be understood to not have the asterisk. If using the asterisk icon, do not leave any space between the label text and the start of the `<svg>` element in the markup. Extra space should not be added in addition to the inline margin.


There's a bit about the "necessity indicator" in the Spectrum guidelines that says you can use text like "(required)" or "(optional)". I didn't see that in the S2 field label Figma file, but it is the s2 field label migration PR.

I've left that off for now, but if we want to add it now, I certainly can. I believe I'd need to add an extra control when required is true, to swap between a text and icon option.

Cool, I was going to ask about this. I think it's fine to only show the asterisk. It might be worth mentioning that you can use text as well though? The docs site uses this wording, maybe something like this?

A Field label for a required field can display either the text “(required)”, or an asterisk.

marissahuysentruyt · 2024-09-25T18:21:54Z

+/**
+ * When the field label is too long for the available horizontal space, it wraps to form another line.
+ */
+export const WrappingAndRequired = Template.bind({});


Do we want to keep this on the docs page, or is it enough to capture it in Chromatic? I think for the most part we've not been showing a wrapping story for components that have wrapping.

If we remove this story from the docs page, I'll keep the sentence about it wrapping, though.

I don't mind it on the docs page, it certainly doesn't hurt to over-communicate in this case!

This is an opinion but I wouldn't mind a control or something (like for instance restricting the width) in Storybook that would allow me to see how Field label wraps, not strictly necessary but maybe nice to have because otherwise the label can hold a lot of text before it'll start to wrap:

Hmm...this is a difficult one for me. It's definitely a little odd that this poor field label is just hanging out by itself, out of context. And I suppose that's my only hesitancy with adding a control, and we can use customStyles if needed (not that customStyles is shown on the docs page or in the controls table). Usually, a field label will be inside of some sort of container or form or something, right? If I go to textfield, the field label wraps at the width of the text field container. The wrapping doesn't seem to be an issue.

I don't have label controls set up for form right now, but if I go into the form.stories.js file and make up a long label, I don't actually see the text wrapping unless I use a ton of text like you did, I change the screen size, or again, pass some customStyles to form.

My gut says we don't need a control for this (namely because this component is just out of context and probably shouldn't be used on its own), but I'm open and can totally add it in if you think it's valuable.

I could also add something like this to the WrappingAndRequired story docs:

"Implementations can pass a maximum width to the field label component using customStyles."

marissahuysentruyt · 2024-09-25T20:28:03Z

+			label: "Interest",
+			id: "form-example-interests",
+			content: [
+				(passthroughs, context) => Fieldgroup({
+					layout: "horizontal",
+					items: [
+						Checkbox({
+							...passthroughs,
+							label: "Kittens",
+							customClasses: ["spectrum-FieldGroup-item"],
+						}, context),
+						Checkbox({
+							...passthroughs,
+							label: "Puppies",
+							customClasses: ["spectrum-FieldGroup-item"],
+						}, context),]
+				}),
+			],


I don't have default items if a user tries to use a form. Should I create some, sort of like what was mentioned for menu?

I don't have a strong opinion on this either way, but it's probably a little smoother if you move these items up to the default args rather than being the args for the Default story, especially since you're reusing them for all the other stories.

castastrophe

Great additions to the test suite and nice alignment with arg naming between stories! Thanks for this update.

rise-erpelding

I left a few opinions and comments, but nothing that would prevent this from being approved!

rise-erpelding · 2024-10-01T17:58:34Z

+RightAligned.storyName = "Right-aligned";
+
+/**
+ * Field labels for required inputs can be marked with an asterisk at the end of the label. Optional inputs would then be understood to not have the asterisk. If using the asterisk icon, do not leave any space between the label text and the start of the `<svg>` element in the markup. Extra space should not be added in addition to the inline margin.


Cool, I was going to ask about this. I think it's fine to only show the asterisk. It might be worth mentioning that you can use text as well though? The docs site uses this wording, maybe something like this?

A Field label for a required field can display either the text “(required)”, or an asterisk.

rise-erpelding · 2024-10-01T18:02:54Z

 			testHeading: "Wrapped",
 			withStates: false,
-			label: "Label example with longer text that will wrap to the next line. And with an asterisk that marks it as required.",
+			label: "Label example with longer text that will wrap to the next line. Sometimes there is an asterisk that marks it as required.",


I didn't see an asterisk, but I wasn't supposed to, right?

Do we want to test it with the asterisk? Or with and without the asterisk?

Correct, we weren't actually testing for the asterisk, but I see how it's confusing now with that text! 🫠 I don't think it's really necessary to test the required state with the wrapping label, but I could be wrong. So just in case, I removed the withStates: false. I also added in ignore: ["Wrapped"] to the disabled state data, so let me know what you think!

It should show the wrapped optional, and wrapped required (with the asterisk) now. 6bbd57f

rise-erpelding · 2024-10-01T18:11:59Z

+/**
+ * When the field label is too long for the available horizontal space, it wraps to form another line.
+ */
+export const WrappingAndRequired = Template.bind({});


This is an opinion but I wouldn't mind a control or something (like for instance restricting the width) in Storybook that would allow me to see how Field label wraps, not strictly necessary but maybe nice to have because otherwise the label can hold a lot of text before it'll start to wrap:

rise-erpelding · 2024-10-01T18:15:40Z

+			label: "Interest",
+			id: "form-example-interests",
+			content: [
+				(passthroughs, context) => Fieldgroup({
+					layout: "horizontal",
+					items: [
+						Checkbox({
+							...passthroughs,
+							label: "Kittens",
+							customClasses: ["spectrum-FieldGroup-item"],
+						}, context),
+						Checkbox({
+							...passthroughs,
+							label: "Puppies",
+							customClasses: ["spectrum-FieldGroup-item"],
+						}, context),]
+				}),
+			],


I don't have a strong opinion on this either way, but it's probably a little smoother if you move these items up to the default args rather than being the args for the Default story, especially since you're reusing them for all the other stories.

- adds missing stories and corresponding documentation - removes duplicate test case for right-aligned - adds customStyles to default test to match all other test styles

- adds missing stories and corresponding documentation - adds new test cases - adds fieldLabelAlignment control for left/right aligned field labels

- added documentation around the "necessity indicator" being text - included a required state for the wrapping/truncation test changes - set default args for form items

marissahuysentruyt force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from 131fded to 753526b Compare September 25, 2024 18:12

marissahuysentruyt commented Sep 25, 2024

View reviewed changes

marissahuysentruyt marked this pull request as ready for review September 25, 2024 18:48

marissahuysentruyt added run_vrt For use on PRs looking to kick off VRT ready-for-review s1 documentation Because documentation is important and shouldn't be broken size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. labels Sep 25, 2024

marissahuysentruyt commented Sep 25, 2024

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from 753526b to afa58be Compare September 26, 2024 13:56

marissahuysentruyt requested review from aramos-adobe, castastrophe, cdransf, jawinn, pfulton and rise-erpelding September 26, 2024 13:58

castastrophe force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from afa58be to c255830 Compare October 1, 2024 16:07

castastrophe approved these changes Oct 1, 2024

View reviewed changes

castastrophe force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from c255830 to 0c9cab2 Compare October 1, 2024 16:11

rise-erpelding approved these changes Oct 1, 2024

View reviewed changes

castastrophe force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from 0c9cab2 to 1ccaef7 Compare October 1, 2024 18:51

pfulton added skip_vrt Add to a PR to skip running VRT (but still pass the action) and removed run_vrt For use on PRs looking to kick off VRT labels Oct 1, 2024

marissahuysentruyt force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from 1ccaef7 to 6bbd57f Compare October 1, 2024 21:18

castastrophe force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from 6bbd57f to d5f66c5 Compare October 2, 2024 13:24

castastrophe added ready-to-merge and removed ready-for-review labels Oct 2, 2024

marissahuysentruyt added 3 commits October 2, 2024 09:40

docs(fieldlabel): adds missing stories and cleans up test cases

cc01846

- adds missing stories and corresponding documentation - removes duplicate test case for right-aligned - adds customStyles to default test to match all other test styles

docs(form): adds missing stories, controls, test cases

d6da6ff

- adds missing stories and corresponding documentation - adds new test cases - adds fieldLabelAlignment control for left/right aligned field labels

chore(fieldlabel,form): pr edits

6c3319c

- added documentation around the "necessity indicator" being text - included a required state for the wrapping/truncation test changes - set default args for form items

castastrophe force-pushed the marissahuysentruyt/css-932-field-label-docs-migration branch from d5f66c5 to 6c3319c Compare October 2, 2024 13:41

castastrophe enabled auto-merge (squash) October 2, 2024 13:41

castastrophe merged commit 6a2c9f2 into main Oct 2, 2024

castastrophe deleted the marissahuysentruyt/css-932-field-label-docs-migration branch October 2, 2024 13:48

This was referenced Mar 12, 2025

fix(checkbox): corrects invalid+checked+hover background color #3617

Merged

test(checkbox): add more coverage for checkbox #3625

Merged

Conversation

marissahuysentruyt commented Sep 25, 2024 • edited by castastrophe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Jira/Specs

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot Bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions Bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

castastrophe left a comment

Choose a reason for hiding this comment

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marissahuysentruyt commented Sep 25, 2024 •

edited by castastrophe

Loading

changeset-bot Bot commented Sep 25, 2024 •

edited

Loading

github-actions Bot commented Sep 25, 2024 •

edited

Loading

github-actions Bot commented Sep 25, 2024 •

edited

Loading

marissahuysentruyt Sep 25, 2024 •

edited

Loading

marissahuysentruyt Sep 25, 2024 •

edited

Loading