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

fix(react): updated textarea counter value changes on re-render #13449

Merged
merged 18 commits into from
May 16, 2023

Conversation

TannerS
Copy link
Contributor

@TannerS TannerS commented Apr 1, 2023

Closes #

#12348

Changelog

New

  • Added more unit test

Changed

  • Added useEffect for value when changed, will update counter

Testing / Reviewing

I actually had to use the sandbox code and manually insert it into the storybook for local testing but its not something that will be testable in storybook

NOTE: will conflict with test file here: https://github.com/carbon-design-system/carbon/pull/12906/files#diff-67daf23818c005abac39307f47f299b05283736cfede71b034c3b2d127f48fcf

@TannerS TannerS requested a review from a team as a code owner April 1, 2023 03:41
@netlify
Copy link

netlify bot commented Apr 1, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4f43820
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6463eacb1b3a0900073137c7
😎 Deploy Preview https://deploy-preview-13449--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 1, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 4f43820
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6463eacbc570690008faf6ea
😎 Deploy Preview https://deploy-preview-13449--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Added this story into the TextArea story file:

export const Test = () => {
  return (<>
  <TextInput
    labelText="TextInputLabel"
    placeholder="Type something here"
    onChange={(e) =>
      (document.getElementById("text-area").value = e.target.value)
    }
  />
  <br></br>
  <br></br>
  <TextArea
    id="text-area"
    labelText="TextAreaLabel"
    maxCount={500}
    enableCounter
    placeholder="Counter should be updated when value is changed by typing something in the TextInput"
  />
</>)
}

and loaded my local storybook and did not see the value update upon typing:
image

@andreancardona
Copy link
Contributor

@TannerS let us know if can address the feedback / review comments :) thanks again for the contribution!

@TannerS
Copy link
Contributor Author

TannerS commented Apr 13, 2023 via email

Co-authored-by: Francine Lucca <40550942+francinelucca@users.noreply.github.com>
@TannerS
Copy link
Contributor Author

TannerS commented Apr 15, 2023

Added this story into the TextArea story file:

export const Test = () => {
  return (<>
  <TextInput
    labelText="TextInputLabel"
    placeholder="Type something here"
    onChange={(e) =>
      (document.getElementById("text-area").value = e.target.value)
    }
  />
  <br></br>
  <br></br>
  <TextArea
    id="text-area"
    labelText="TextAreaLabel"
    maxCount={500}
    enableCounter
    placeholder="Counter should be updated when value is changed by typing something in the TextInput"
  />
</>)
}

and loaded my local storybook and did not see the value update upon typing: image

hello, i can shoe u over a call if you want, i lost some of my code but i can re-create it, i did it going off this #12348 (comment) since i dont think updating the dom directly is possible to trigger anything

@francinelucca
Copy link
Collaborator

hello, i can shoe u over a call if you want, i lost some of my code but i can re-create it, i did it going off this #12348 (comment) since i dont think updating the dom directly is possible to trigger anything

You're right @TannerS , thanks for clearing that up. I tried it with useState and it works!

export const Test = () => {
  const [text, setText] = useState('');
  return (
    <>
      <TextInput
        labelText="TextInputLabel"
        placeholder="Type something here"
        onChange={(e) => setText(e.target.value)}
      />
      <br></br>
      <br></br>
      <TextArea
        id="text-area"
        labelText="TextAreaLabel"
        maxCount={500}
        enableCounter
        placeholder="Counter should be updated when value is changed by typing something in the TextInput"
        value={text}
      />
    </>
  );
}

I did find one bug in which the counter gets updated when attempting to type into the textArea directly in this scenario , the counter changes but the text does not (expected but the text but not for the counter). See attached screen record

Screen.Recording.2023-04-17.at.2.15.50.PM.mov

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

@TannerS
Copy link
Contributor Author

TannerS commented May 3, 2023

sorry guys this is a bit old i been gone but now i can continue, is there anything else needed on my end? @francinelucca @andreancardona

@andreancardona
Copy link
Contributor

@TannerS were you able to address Francine's comment? If so, I can take a look and test locally :)

@TannerS
Copy link
Contributor Author

TannerS commented May 8, 2023 via email

@francinelucca
Copy link
Collaborator

I think I did...if not I can go over stuff in an office hours Sent from my T-Mobile 5G Device Get Outlook for Androidhttps://aka.ms/AAb9ysg

________________________________ From: Andrea N. Cardona @.> Sent: Monday, May 8, 2023 12:29:12 PM To: carbon-design-system/carbon @.> Cc: Tanner Summers @.>; Mention @.> Subject: Re: [carbon-design-system/carbon] fix(react): updated textarea counter value changes on re-render (PR #13449) @TannerShttps://github.com/TannerS were you able to address Francine's comment? If so, I can take a look and test locally :) — Reply to this email directly, view it on GitHub<#13449 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACDUUD3PZU6SFHAKXHPNTIDXFEUORANCNFSM6AAAAAAWPLSYXM. You are receiving this because you were mentioned.Message ID: @.***>

I'm still seeing the error referenced here when running locally, happy to discuss further!

@francinelucca
Copy link
Collaborator

francinelucca commented May 15, 2023

Met with @TannerS and opted to add a settimeout function before overriding the text counter in the useEffect to also solve for this bug

Doing this because the textArea's onChange property doesn't get re-triggered when the value is later re-written to match the textInput value. Delaying the textCount assignation allows for the textarea element value to catchup before the textcount is calculated

This should be ready for review @andreancardona

- use value,defaultValue in useEffect insead of ref
- delay textCount assignation
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

reviewed and looks good!

@kodiakhq
Copy link
Contributor

kodiakhq bot commented May 16, 2023

This PR could not be merged because the GitHub API returned an internal server error. To enable Kodiak on this pull request please remove the kodiak:disabled label.

When the GitHub API returns an internal server error (HTTP status code 500), it is not safe for Kodiak to retry merging.

For more information please see https://kodiakhq.com/docs/troubleshooting#merge-errors

If you need help, you can open a GitHub issue, check the docs, or reach us privately at support@kodiakhq.com.

docs | dashboard | support

@kodiakhq kodiakhq bot merged commit ae85de9 into carbon-design-system:main May 16, 2023
SunnyJohal pushed a commit to SunnyJohal/carbon that referenced this pull request May 23, 2023
…on-design-system#13449)

* fix(react): updated textarea counter value changes on re-render

* fix: format

* Update packages/react/src/components/TextArea/__tests__/TextArea-test.js

* Update packages/react/src/components/TextArea/__tests__/TextArea-test.js

* Update packages/react/src/components/TextArea/__tests__/TextArea-test.js

* Update packages/react/src/components/TextArea/__tests__/TextArea-test.js

Co-authored-by: Francine Lucca <40550942+francinelucca@users.noreply.github.com>

* fix(TextArea): use textarea ref value instead of [value,defaultValue]

* fix: format

* fix(TextArea): add value to textCounter dependency array

* fix(TextArea): textCount optimizations

- use value,defaultValue in useEffect insead of ref
- delay textCount assignation

* fix: format

---------

Co-authored-by: Francine Lucca <francinelucca@hotmail.com>
Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
Co-authored-by: Francine Lucca <40550942+francinelucca@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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.

3 participants