Skip to content
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.

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

feat: input mask #2538

Merged
merged 12 commits into from
Aug 24, 2023
Merged

feat: input mask #2538

merged 12 commits into from
Aug 24, 2023

Conversation

werdnanoslen
Copy link
Contributor

@werdnanoslen werdnanoslen commented Jul 31, 2023

Summary

Adds input masks to text inputs

Related Issues or PRs

Resolves #2517

How To Test

Open the last story for Text input: Input Mask

Screenshots (optional)

This is me just pianoing 1-9, showing that it autoformats:
Aug-02-2023 13-53-06

@werdnanoslen werdnanoslen marked this pull request as ready for review August 2, 2023 20:54
@werdnanoslen werdnanoslen requested a review from a user August 2, 2023 20:55
@shkeating
Copy link
Contributor

shkeating commented Aug 3, 2023

looks good to me

whoops, I overlooked an ANDI warning we might want investigate

image

I'm seeing it suggest using aria-labelledby instead of aria-describedby

@shkeating shkeating self-requested a review August 3, 2023 00:02
shkeating
shkeating previously approved these changes Aug 3, 2023
@werdnanoslen
Copy link
Contributor Author

werdnanoslen commented Aug 3, 2023

looks good to me

whoops, I overlooked an ANDI warning we might want investigate
image

I'm seeing it suggest using aria-labelledby instead of aria-describedby

It already has a label though (associated with for=), and describedby is supposed to be for extra info on top of a label. Also USWDS uses describedby

I'll just add aria-labelledby also to point to the label:
image

Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme know if you have any questions about this feedback!

src/components/forms/TextInput/TextInput.test.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInput.test.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInput.test.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInput.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInput.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInput.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInput.tsx Outdated Show resolved Hide resolved
shkeating
shkeating previously approved these changes Aug 9, 2023
@werdnanoslen
Copy link
Contributor Author

@gidjin once this is merged, could we please do a minor release to make this available for my current client?

Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as not to block.

However, I would like to see a reorg (can be a follow on PR if releasing this is priority), where the folder structure looks like:

src/
  components/
    forms/
      TextInput/
        {...all our existing stuff}
      TextInputMask/
        TextInputMask.tsx
        TextInputMask.test.tsx
        TextInputMask.stories.tsx

Basically just to make sure that the tests and storybook examples are also separated out

Edit: reversing course, noted a few small things that I think are actually functionally important

src/components/forms/TextInput/TextInputMask.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInputMask.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInputMask.tsx Outdated Show resolved Hide resolved
src/components/forms/TextInput/TextInputMask.tsx Outdated Show resolved Hide resolved
className="usa-input-mask--content"
aria-hidden
data-testid={`${id}Mask`}>
<i>{iValue}</i>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll reiterate that if you want the <i> tag to not show up until something has been typed (like the USDWS examples) you can do this

Suggested change
<i>{iValue}</i>
{iValue && <i>{iValue}</i>}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, but i didn't see any reason to 🤷

@werdnanoslen werdnanoslen merged commit 9d2fe14 into main Aug 24, 2023
@werdnanoslen werdnanoslen deleted the an-inputmask-2517 branch August 24, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Component: Input Mask
3 participants