New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WNMGDS-2843] Add `ds-text-field` component #3172

Merged

pwolfert merged 18 commits into main from pwolfert/ds-text-field

Jul 26, 2024

Contributor

pwolfert commented Jul 25, 2024

Summary

Adds a new ds-text-field web component.

How to test

Try it out in storybook
If you really want to, you can try adding it to the astro example too, but I did that myself already and removed it because I didn't have a good use case for it in the example, but maybe that's lazy and I should actually just revamp the example page and make a more meaningful form example that includes more components.

Checklist

Prefixed the PR title with the Jira ticket number as [WNMGDS-####] Title or [NO-TICKET] if this is unticketed work.
Selected appropriate Type (only one) label for this PR, if it is a breaking change, label should only be Type: Breaking
Selected appropriate Impacts, multiple can be selected.
Selected appropriate release milestone

If this is a change to code:

Created or updated unit tests to cover any new or modified code
If necessary, updated unit-test snapshots (yarn test:unit:update) and browser-test snapshots (yarn test:browser:update)

If this is a change to documentation:

Checked for spelling and grammatical errors

pwolfert added 7 commits

July 17, 2024 12:07


          Add basics of new ds-text-field component

84a0d7c


          Add autofocus since we added it to ds-droppdown

9c18128


          Add some unit tests

4fb3eb9


          Make the built-in option strings easier on the eyes and add a label-m…

860576d

…ask test


          Add it to the index

df184f0


          Make sure it renders as block

a13a380


          Merge branch 'main' into pwolfert/ds-text-field

60fdb6d

pwolfert added Impacts: Core Type: Added labels

pwolfert added this to the 11.0.0-beta.1 milestone

pwolfert requested review from zarahzachz, kim-cmsds, tamara-corbalt and jack-ryan-nava-pbc

July 25, 2024 17:32


          Add browser test snapshots

af0fbc1

jack-ryan-nava-pbc requested changes

View reviewed changes

Collaborator

jack-ryan-nava-pbc left a comment

Overall this looks great! Mostly just some discrepancies I noticed in the Story plus some questions about how some props (well, actually just one prop) are/is being passed in.

...system/src/components/web-components/ds-text-field/__snapshots__/ds-text-field.test.tsx.snap Show resolved Hide resolved

...system/src/components/web-components/ds-text-field/__snapshots__/ds-text-field.test.tsx.snap Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.tsx Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx

+                  },
+                  'error-placement': {
+                    description: 'Location of the error message relative to the field input',
+                    options: [undefined, 'top', 'bottom'],

Collaborator

jack-ryan-nava-pbc Jul 25, 2024

Nit: If it's undefined the default behavior is to place the location on top, can/should this detail be captured here?

Contributor Author

pwolfert Jul 25, 2024

By "captured here" do you mean explaining that in the description?

Contributor Author

pwolfert Jul 25, 2024

(Default position is different for different child design systems.)

Collaborator

jack-ryan-nava-pbc Jul 25, 2024

Yep! I do mean explaining that in the description.

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

pwolfert added 2 commits

July 25, 2024 13:53


          Fix attribute docs according to feedback

979b066


          Merge branch 'main' into pwolfert/ds-text-field

fc975fd

kim-cmsds approved these changes

View reviewed changes

Collaborator

kim-cmsds left a comment

Nice, this looks great to me! I left a few non-blocking questions.

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.stories.tsx Outdated Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.test.tsx Show resolved Hide resolved

zarahzachz approved these changes

View reviewed changes

Collaborator

zarahzachz left a comment

Looks good, just some small comments you can disregard or accept

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.test.tsx Show resolved Hide resolved

packages/design-system/src/components/web-components/ds-text-field/ds-text-field.test.tsx Outdated

+                  expect(input).not.toHaveAttribute('disabled');
+                });
+                it('applies additional classes to the wrapper', () => {

Collaborator

zarahzachz Jul 26, 2024

The following is probably more philosophical than the scope of this ticket warrants. Feel free to disregard my stream-of-conscious take here:

This test initially confused me because I associate "wrapper" with the host for 🚾, and having a special prop to apply a class to a host is unnecessary because it's HTML and class would work fine.

Anyways... I had to do a double-take and read what the test was actually doing and realized "wrapper" means something different because it's querying the clearfix style in the test, which is the internal div within the host wrapper. Is this div even needed? Who clearfixes in the era of flexbox and grid?

Do we need better language when talking about these sorts of tags?

Contributor Author

pwolfert Jul 26, 2024

We might very well be able to remove that "wrapper" div now. I agree that it's beyond the scope here, but it's worth considering for the future (@kim-cmsds).

That's a great point about the wording here being confusing. I think I'll change it to "root element" or something like that.

pwolfert added 3 commits

July 26, 2024 09:57


          Fix possibility of passing both autofocus and autoFocus to Preact…

54c0cad

… component


          Added description to requirement-label

5faed7f


          Rewrite label mask prop/attr descriptions and fix broken links in Sto…

7471e35

…rybook

pwolfert commented

View reviewed changes

packages/design-system/src/components/web-components/ds-choice/ds-choice.stories.tsx Outdated

Comment on lines 98 to 104

-                    description:
-                      'Text showing the requirement (ie. "Optional", or "Required").\nIn most cases, this should be used to indicate which fields are optional.\nSee the [form guidelines](https://design.cms.gov/patterns/Forms/forms/) for more info.',
+                    description: (
+                      <>
+                        Text showing the requirement (ie. &quot;Optional&quot;, or &quot;Required&quot;).\nIn most
+                        cases, this should be used to indicate which fields are optional.\nSee the{' '}
+                        <a href="https://design.cms.gov/patterns/Forms/forms/">form guidelines</a> for more info.
+                      </>
+                    ),

Contributor Author

pwolfert Jul 26, 2024

Turns out this change fixes the doc pages but breaks the story pages (probably an error when rendering the Storybook controls). I think Storybook must not like rendering non-strings in the control descriptions.


          Undo the change to make descriptions with links React elements

803f194

This was fine on our docs page with our custom attributes table, but Storybook crashed trying to render these values in story controls. The error was completely unhelpful and the stack trace was all through minified code, so I had to guess and test to find the problem here. Now we know these can only be strings.

jack-ryan-nava-pbc approved these changes

View reviewed changes

Collaborator

jack-ryan-nava-pbc left a comment

LGTM!

pwolfert added 4 commits

July 26, 2024 11:14


          Add support for multiline fields and a unit test for it

723ccf2


          Don't use the name "wrapper" because it can be confused with the WC w…

d005506

…rapper

And honestly we might not even need this element anymore. Sarah brought that up, and it's worth considering.


          Accidentally left this in, which messes up VRTs

64e0b2a


          Add snapshots for the new multi-line story

0db1820

pwolfert merged commit a15cb15 into main

1 check passed

pwolfert deleted the pwolfert/ds-text-field branch

July 26, 2024 21:06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Impacts: Core Type: Added