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

v3 - custom string input with character count #529

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

mathiazom
Copy link
Contributor

@mathiazom mathiazom commented Aug 27, 2024

Further explanation is found here: #527

Composes the default string input rendering with a simple character count, with required max count. Added to most of the fields with type: "string", but can also be expanded to other fields later.

Also renamed all instances of the validation function argument Rule to rule (since this is a variable and not a type or class)

Visual Overview (Image/Video)

image image image

Checklist

Please ensure that you’ve completed the following checkpoints before submitting your pull request:

  • Documentation: Relevant documentation has been added or updated (if applicable).
  • Testing: Have you tested your changes thoroughly?
    • Please list the types of tests you've run (unit, integration, manual, etc.):

Additional Notes

Other comments relevant to this pull request.

Copy link

vercel bot commented Aug 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
variant-no ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 13, 2024 7:01am
variant-se-2 ✅ Ready (Inspect) Visit Preview Sep 13, 2024 7:01am

Copy link

@christinaroise christinaroise left a comment

Choose a reason for hiding this comment

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

I know this didn't take too long, but it'd be nice if we followed Monday Comittments more strictly in terms of not picking tasks outside of our goals.

I also believe there are several other places in the repository with character limit in need of the count.

@mathiazom
Copy link
Contributor Author

I know this didn't take too long, but it'd be nice if we followed Monday Comittments more strictly in terms of not picking tasks outside of our goals.

I agree, but my other tasks were blocked and this was almost finished from last week.

@mathiazom
Copy link
Contributor Author

I also believe there are several other places in the repository with character limit in need of the count.

Should these be added in this PR or left for later?

@christinaroise
Copy link

I agree, but my other tasks were blocked and this was almost finished from last week.

I noticed after some digging on slack! We need a better system in terms of alerting others when we're blocked. Especially when we're working on tasks towards out goal for the week.

@christinaroise
Copy link

Should these be added in this PR or left for later?

I'd do them all in the same PR 👍

@christinaroise
Copy link

@mathiazom maybe convert to draft until you have the time to add char.limit to the remaining places?

@mathiazom mathiazom marked this pull request as draft September 3, 2024 09:28
@mathiazom mathiazom marked this pull request as ready for review September 11, 2024 06:11
@@ -0,0 +1,26 @@
import { Box, Stack, Text } from "@sanity/ui";

Choose a reason for hiding this comment

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

before we merge can you make sure it passes wcag 2.1? (plenty of studio is just bleh, but we can give it a try since it's a custom component
https://designsystem.digital.gov/components/character-count/accessibility-tests/

Copy link
Contributor Author

@mathiazom mathiazom Sep 13, 2024

Choose a reason for hiding this comment

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

Updated now to show the remaining characters, with a red text when over limit. Also added aria-describedby to announce allowed number of characters. aria-live has been set on the counter, but I have struggled to get the screen reader to announce it reliably. Seems to be intercepted by text field description, which might be Sanity's fault. Default text input is slightly more reliable based on quick testing.

Choose a reason for hiding this comment

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

Yeah, I believe this is on Sanity. Great job though!

@mathiazom mathiazom merged commit ef6932d into v3 Sep 13, 2024
6 checks passed
@mathiazom mathiazom deleted the feat/v3-text-field-char-count branch September 13, 2024 07:18
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.

3 participants