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(textarea): added char counter #10442

Merged
merged 21 commits into from
Feb 7, 2022

Conversation

TannerS
Copy link
Contributor

@TannerS TannerS commented Jan 13, 2022

Closes #

#1672
#5702

Changelog

New

Adds two new props to enable a textarea counter, and an actually max counter limit. Added some styles too.

Testing / Reviewing

Added functionality to test live changes in storybook and added unit test. I am all for people helping me look over my unit test for feedback

{{ Add descriptions, steps or a checklist for how reviewers can verify this PR works or not }}

Added

  1. Styles
  2. Coded changes (2 props)

Behavior

  1. Ability to enable and disable the counter and re-render
  2. Follows the text areas maxlength attribute so the user can only type up until it hits the max then does not allow anymore (no error validation option)
  3. both props have to be enabled for this to work. I originally thought of just adding a counter prop which will turn on the counter but I then thought to give the user the ability to turn it off and on without having to erase the counter data BUT this can be discussed in this PR
  4. The counter should use the same styles as the label to look the same

Test

  1. Added unit test. I did have issues finding a way to write a test to grab the value after adding text. Even after looking at many ways online so my test may be limited.

@TannerS TannerS requested review from a team as code owners January 13, 2022 21:28
@netlify
Copy link

netlify bot commented Jan 13, 2022

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: dbe11e4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6201646d3fa84f00085e6ab3

😎 Browse the preview: https://deploy-preview-10442--carbon-react-next.netlify.app

@TannerS
Copy link
Contributor Author

TannerS commented Jan 13, 2022

@abbeyhrt fyi

@netlify
Copy link

netlify bot commented Jan 13, 2022

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: dbe11e4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6201646df159090008365293

😎 Browse the preview: https://deploy-preview-10442--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jan 13, 2022

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: dbe11e4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6201646d2e28bb00072ee931

😎 Browse the preview: https://deploy-preview-10442--carbon-components-react.netlify.app

@TannerS TannerS requested a review from abbeyhrt January 25, 2022 05:29
@TannerS TannerS changed the title Tanner text area counter Add counter to TextArea Jan 25, 2022
@TannerS TannerS requested a review from tw15egan January 25, 2022 16:51
@TannerS
Copy link
Contributor Author

TannerS commented Jan 25, 2022

@tw15egan updated

@TannerS
Copy link
Contributor Author

TannerS commented Feb 2, 2022

@tw15egan @abbeyhrt my PR failed a unit test unrelated to my code, some timeout error, how do re-run the test check?

@tw15egan
Copy link
Collaborator

tw15egan commented Feb 2, 2022

@TannerS can you try running yarn test -u at the root, add any changes, and push it up?

@TannerS
Copy link
Contributor Author

TannerS commented Feb 2, 2022

@tw15egan thanks! ya that updated a file.

I am a big confused, what does that command do? it seemed to update by adding the prop autoOrientation to this file packages/react/src/components/Tooltip/Tooltip.js

which is part of an active PR https://github.com/carbon-design-system/carbon/pull/10611/files

so I am a bit confused on what that commands does and why did it add that prop that was added to a PR not yet merged?

@tw15egan
Copy link
Collaborator

tw15egan commented Feb 2, 2022

yarn test -u just updates the snapshot tests, not sure why it would pull in code from a different PR

@TannerS
Copy link
Contributor Author

TannerS commented Feb 2, 2022

yarn test -u just updates the snapshot tests, not sure why it would pull in code from a different PR

@tw15egan I thought maybe I messed up a merge or something but it added that prop without the eslint comment like in the PR above, so I am not sure...

either way, it may been what I needed to fix it, so thank you!

@TannerS
Copy link
Contributor Author

TannerS commented Feb 2, 2022

yarn test -u just updates the snapshot tests, not sure why it would pull in code from a different PR

ok, maybe i just messed up and the merge i did left out a file, either way, it should be fixed, so thanks!

@abbeyhrt abbeyhrt requested review from a team and laurenmrice and removed request for a team February 3, 2022 17:00
@abbeyhrt
Copy link
Contributor

abbeyhrt commented Feb 3, 2022

@TannerS This looks great so far! One thing I noticed, messing around with the story, is that you can set a maxCount when enableCounter is false and it does prevent the user from typing passed that maxCount. Is that intended? It seems a bit weird to have a limit that isn't communicated to the user but there definitely could be a use case you have in mind that I don't :)

@laurenmrice
Copy link
Member

Visually it looks good to me. Thank you! 👍. But I also have the same question as Abbey above^

@TannerS
Copy link
Contributor Author

TannerS commented Feb 3, 2022 via email

@TannerS
Copy link
Contributor Author

TannerS commented Feb 3, 2022

@abbeyhrt @laurenmrice hey guys, ya it was a bug, I pushed a fix, let me know if the way I fixed it is acceptable / carbon standards !

Comment on lines +57 to +60
if (enableCounter) {
textareaProps.maxLength = maxCount;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

🎉 🎉 Looks great to me! Thank you for the contribution 😃

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for contributing this! ✅ ⭐️

@kodiakhq kodiakhq bot merged commit 2fa6584 into carbon-design-system:main Feb 7, 2022
@TannerS TannerS changed the title Add counter to TextArea feat(textarea): added char counter Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants