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: set value attribute on textarea instead of in content to ensure value and defaultValue stay in sync with changes #447

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kmillns
Copy link

@kmillns kmillns commented Nov 25, 2024

🚀 Description

Fixes issue #446 where the underlying <textarea> used in Form::Controls::Textarea gets out of sync with @value changes after typing in the <textarea>.

Also adds tests for both current working functionality of updating value before typing in the textarea and the previously broken case of typing in the textarea and then attempting an update of the value from outside the component.


🔬 How to Test

Added test cases should cover the functionality changing.

There's no external impact on existing examples to show.

Reversing the change in textarea.gts should result in one failed test case for the issue described in #446

…value and defaultValue stay in sync with changes
Copy link

changeset-bot bot commented Nov 25, 2024

🦋 Changeset detected

Latest commit: 34df8d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@crowdstrike/ember-toucan-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@nicolechung nicolechung left a comment

Choose a reason for hiding this comment

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

Thank you for making this fix 🚀

@nicolechung
Copy link
Contributor

Screenshot 2024-11-26 at 1 59 06 PM

@kmillns I'm a little confused as I am unable to re-create the error problem over here: https://ember-toucan-core.pages.dev/docs/components/textarea

But your code and test look correct to me, so I've approved.

@kmillns
Copy link
Author

kmillns commented Nov 26, 2024

Screenshot 2024-11-26 at 1 59 06 PM @kmillns I'm a little confused as I am unable to re-create the error problem over here: https://ember-toucan-core.pages.dev/docs/components/textarea

But your code and test look correct to me, so I've approved.

Yeah, I don't think it's something that would normally happen with just typing in the <textarea> but instead is the combination of typing, which causes the underlying <textarea> to use .value instead of .defaultValue and then triggering a change in the @value, which is essentially just changing the .defaultValue.

These screenshots show how .value and .defaultValue (initially both '' in the demo) get out of sync after typing in the <textarea>, and the @value being inside only updates the .defaultValue so it will only appear out of sync if trying to update after typing.
Screenshot 2024-11-26 at 3 35 38 PM
Screenshot 2024-11-26 at 3 35 54 PM
Screenshot 2024-11-26 at 3 36 09 PM

@@ -80,7 +80,8 @@ export default class ToucanFormTextareaControlComponent extends Component<Toucan
readonly={{@isReadOnly}}
...attributes
{{on "input" this.handleInput}}
>{{@value}}</textarea>
Copy link
Contributor

Choose a reason for hiding this comment

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

@simonihmig I can't recall exactly why we did this initially. Do you?

Copy link
Contributor

@ynotdraw ynotdraw left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's see if @simonihmig remembers about the value change above to make sure we don't have a potential issue before merging! Thanks!

@ynotdraw
Copy link
Contributor

@kmillns I kicked off CI and looks like a few linting/TS errors

@kmillns
Copy link
Author

kmillns commented Dec 2, 2024

Everything looks green except cloudflare publish which I think is unrelated to this PR:

Error: Input required and not supplied: apiToken

@ynotdraw
Copy link
Contributor

ynotdraw commented Dec 2, 2024

Everything looks green except cloudflare publish which I think is unrelated to this PR:

Error: Input required and not supplied: apiToken

Yeah, that'll fail since you aren't in the org. Only outstanding thing to me is #447 (review). @simonihmig do you recall?

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