Skip to content

fix: fix text area auto-expand when controlled #912

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

Merged
merged 3 commits into from
Apr 30, 2025
Merged

Conversation

gnapse
Copy link
Contributor

@gnapse gnapse commented Apr 29, 2025

Short description

Fixes #911

The main issue is that our current auto-expand implementation is more suitable for when the component is uncontrolled. When the component is controlled, it is better to use the value changes as the signal to run the effect.

This PR now changes the auto-expand to set up two different useEffect calls:

  1. one that runs only when the component is uncontrolled (it uses the oninput event to update the auto-expand size)
  2. one that runs only when the component is controlled (it has the value as dependency in the effect, to update the auto-expand size)

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Reviewed and approved Chromatic visual regression tests in CI

@gnapse gnapse requested a review from a team April 29, 2025 13:33
@gnapse gnapse self-assigned this Apr 29, 2025
@gnapse gnapse requested review from engfragui and Copilot and removed request for a team April 29, 2025 13:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the text area auto-expand functionality for controlled components by splitting the logic into two useEffect calls within a new useAutoExpand hook.

  • Introduces a new useAutoExpand hook to handle controlled and uncontrolled auto-expand behavior.
  • Removes the previous unified auto-expand useEffect in favor of the new split implementation.
Files not reviewed (1)
  • src/text-area/text-area.stories.mdx: Language not supported

{...props}
label="What do you want to accomplish?"
message="This one will not auto-expand."
autoExpand={false}
Copy link
Contributor

@engfragui engfragui Apr 30, 2025

Choose a reason for hiding this comment

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

💭 Was it intentional to remove the "What do you want to accomplish?" textarea that had autoExpand={false}? I'm ok without it (as anyway there are plenty of other stories with textareas with autoExpand={false}, so not really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my thinking too.

event.preventDefault()
setValue('') // Clear the input programmatically
}
}}
Copy link
Contributor

@engfragui engfragui Apr 30, 2025

Choose a reason for hiding this comment

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

💭 I've noticed that this textarea extends further than before when I look at it in storybook (compared to prod). Not sure if it means anything interesting though 😅

Before (prod)After (this PR)

Screenshot 2025-04-30 at 11 18 47

Screenshot 2025-04-30 at 11 18 38

Copy link
Contributor

@engfragui engfragui Apr 30, 2025

Choose a reason for hiding this comment

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

Mmm it seems to be because of the different max-width value compared to before (prod):

Before (prod)After (this PR)

Screenshot 2025-04-30 at 11 23 52

Screenshot 2025-04-30 at 11 23 17

Copy link
Contributor

@engfragui engfragui Apr 30, 2025

Choose a reason for hiding this comment

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

❌ Oh I guess it's because we're not passing {...props} anymore to the textarea in this story. Assuming it wasn't intentional, I think we should put it back so that devs can play with the story in the Canvas like before, right? (Otherwise the controls would not work)

Before (prod)After (this PR)
CleanShot.2025-04-30.at.11.34.50.mp4
CleanShot.2025-04-30.at.11.35.27.mp4

Copy link
Contributor Author

@gnapse gnapse Apr 30, 2025

Choose a reason for hiding this comment

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

My mistake! Must have removed the {...props} inadvertently. Will fix it.

const isControlled = value !== undefined

React.useEffect(
function setupAutoExpandWhenUncontrolled() {
Copy link
Contributor

@engfragui engfragui Apr 30, 2025

Choose a reason for hiding this comment

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

💭 I see that in setupAutoExpandWhenControlled you do !isControlled -> return.
Would it make sense to do isControl -> return in this case? 🤔

Copy link
Contributor

@engfragui engfragui left a comment

Choose a reason for hiding this comment

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

Makes sense to me overall! I left one blocking comment (need to pass {...props} to the controlled textarea in storybook).

@gnapse gnapse merged commit 7d33744 into main Apr 30, 2025
6 checks passed
@gnapse gnapse deleted the ernesto/fix-auto-expand branch April 30, 2025 13:00
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.

TextArea with autoExpand does not update the height when the value is programmatically changed
2 participants