spectrum-css icon indicating copy to clipboard operation
spectrum-css copied to clipboard

feat(textfield): migrate to spectrum 2

Open rise-erpelding opened this issue 1 year ago • 3 comments

Description

CSS-767

This work migrates the Textfield component to Spectrum 2:

Migration Work

  • Medium sizing is now the default
  • The textfield component was checked against the tokens design spec and any discrepancies were adjusted
    • Note that there are some tokens that currently have different values than what is seen in Figma, this is (for the moment) intentional - see notes in Figma
  • New tokens added to match design spec
  • Quiet variant removed

Fixes

  • Adds vendor prefix updates to textfield CSS from main (see #2771) to minimize conflict (as more of a temporary commit)
  • Makes text field keyboard focusable where it was not before
  • Improves the disabled state in WHCM to look different from default state

Storybook Updates

  • Adds character count, placeholder, and helptext as controls.
  • Removes Text Area story in favor of showing under Default story
  • Lightly increases Chromatic coverage

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

  1. Check the Storybook deployment preview for textfield:
  2. Confirm that code style matches S2: @marissahuysentruyt
    • [x] Spectrum styles have been moved to index.css
    • [x] Medium is default
  3. Confirm that quiet variant is removed: @marissahuysentruyt
    • [x] There is no remaining evidence of quiet variant in CSS or in implementation (Storybook)
  4. Check that adjustments to design match spec: @marissahuysentruyt
    • [x] Spacing start/end edge to value for XL component matches design (should be using component-edge-to-text-300)
    • [x] Spacing for top edge to value text should change with size to match spec instead of remaining the same for every t-shirt size
    • [x] Spacing for bottom edge to value text should change with size to match spec instead of remaining the same for every t-shirt size, note that the token values don't match the Figma value, and you can confirm the values here if needed
    • [x] Border thickness matches spec
    • [x] Border colors match spec
    • [x] Inline side label variant has --spectrum-spacing-200 for every size
    • [x] Disabled background color (text input color) and border color match design spec
    • [x] Font tokens match spec
    • [x] Check multiline (textarea) as well as text input
  5. New additions to Storybook function as expected to display the components and add controls @marissahuysentruyt
    • [x] Character count/Has character count control
    • [x] Placeholder
    • [x] Help text
    • [x] Any controls missing that would be helpful to have, or any not working as expected?
  6. Chromatic testing preview provides additional coverage @marissahuysentruyt
    • [x] Test cases are added as needed
  7. Various states, modes, and variants behave as expected (note that this has not yet undergone design review and there may be some minor adjustments, but definitely feel free to call out anything that would be good to ask/confirm!)
    • [x] Various combinations of elements/components and states work as expected - including read-only, disabled, placeholder, character count, helptext, etc.
    • [ ] Read-only styling should win over invalid if both are applied
    • [ ] Disabled styling should win over read-only if both are applied
    • [ ] Windows High Contrast Mode looks as expected

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • [X] The pages render correctly, are accessible, and are responsive.

Screenshots

Before S2 migration: image

After S2 migration (updated 7/22/24): image

To-do list

  • [X] I have read the contribution guidelines.
  • [X] I have updated relevant storybook stories and templates.
  • [X] I have tested these changes in Windows High Contrast mode.
  • [X] If my change impacts other components, I have tested to make sure they don't break.
  • [X] If my change impacts documentation, I have updated the documentation accordingly.
  • [ ] Updated S2 Icons have been added
  • [ ] Design review/approval
  • [ ] Code review/approval
  • [ ] ✨ This pull request is ready to merge. ✨

rise-erpelding avatar Jun 21 '24 22:06 rise-erpelding

🦋 Changeset detected

Latest commit: dcc9e1a4bf791840a42aedf69de547616494445c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/textfield Major

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

changeset-bot[bot] avatar Jun 21 '24 22:06 changeset-bot[bot]

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

github-actions[bot] avatar Jun 21 '24 22:06 github-actions[bot]

File metrics

Summary

Total size: 1.37 MB*

Package Size Minified Gzipped
textfield 31.83 KB 30.42 KB 3.60 KB

textfield

Filename Head Minified Gzipped Compared to base
index.css 31.83 KB 30.42 KB 3.60 KB 🟢 ⬇ 0.64 KB
metadata.json 17.01 KB - - 🔴 ⬆ 0.06 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions[bot] avatar Jun 21 '24 22:06 github-actions[bot]

@marissahuysentruyt

A couple more questions I couldn't really find a good spot for: displayLabel = false: do you think that should be changed to true in the template.js args? I think the Figma component has the label displayed by default, so should we update that at all?

I updated this!

required: do you think we should add any documentation about the required attribute being added with the control? When a user toggles that required control, visually they don't see anything, so could we add something about the required attribute in the control description? We could add a required story too if you wanted. 🤷‍♀️

This was a good catch! I'd previously had this conversation with the designer and subsequently realized it was just an issue in the template where I wasn't passing that required prop down to FieldLabel. I'm not sure if I only fixed it in my head or if I made the change and it was lost in a rebase, but in any case we should have asterisks now! I also added a description for the control and made some "Required" stories. According to the guidelines, you can use the asterisk or add "(required)" into the label. IMO, I don't think it's worthwhile to test "(required)" or make it part of controls, but I did address it and show it on the Docs page, let me know if you think that's sufficient.

rise-erpelding avatar Feb 25 '25 18:02 rise-erpelding

Latest updates should address the most recent comments/changes requested @marissahuysentruyt @jawinn! They include:

  • Docs page:
    • Removing the keyboard focused story and combining its comments into the default-level story.
    • Removing invalid states from the default-level story and making them their own story.
    • Minor reformatting of JSDoc comments, including addition of line breaks for all long comments (there had already been a few with line breaks, so this adds consistency), removal of curly quotes in favor of straight quotes, and addition of references to textfield from textarea and vice versa.
    • Removal of wrapper and container spacing styles, stories with multiple containers displayed in row rather than column direction
  • Storybook controls
    • Hiding isLoading control because it looks unexpected and is not in the design spec (I will create a ticket for this)
    • Add descriptions for select controls
  • Testing template: add disabled & hover tests
  • CSS: removal of :focus-visible selectors as they cause the focus outline to appear on mouse focus

rise-erpelding avatar Mar 14 '25 16:03 rise-erpelding