Added fix for properties textarea-rows and textarea-width issues#2035
Added fix for properties textarea-rows and textarea-width issues#2035varun-srinivasa wants to merge 29 commits intosiemens:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 843103f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
Missing component test which verify all cases. Please add
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix issues with the textarea-rows and textarea-width properties. The approach of removing the fixed height from the textarea mixin and using CSS custom properties for the width is sound. The added regression tests are also a great addition to verify the fixes.
I've found a couple of issues with the implementation. The logic for textareaWidth does not account for dynamic updates to the property after the component has loaded. Additionally, there's a conflicting inline style being applied that will prevent the new width logic from working as intended. I've also pointed out a minor code style inconsistency in the SCSS changes. My review comments provide more details and suggestions for how to address these points.
| if (this.textareaWidth) { | ||
| this.hostElement.style.setProperty( | ||
| '--textarea-width', | ||
| this.textareaWidth | ||
| ); | ||
| this.hostElement.setAttribute('textarea-width', ''); | ||
| } |
There was a problem hiding this comment.
There are a couple of issues with the implementation for textareaWidth:
-
Dynamic updates are not handled: This logic only runs when the component loads. If the
textareaWidthprop is changed dynamically after the component has been mounted, the width will not update. To fix this, you should handle property changes using a@Watch('textareaWidth')decorator. I suggest extracting this logic into a private method and calling it from bothcomponentWillLoadand the new watcher method.Example:
@Watch('textareaWidth') onTextareaWidthChange() { this.setTextareaWidth(); } // In componentWillLoad, replace the current logic with: this.setTextareaWidth(); private setTextareaWidth() { if (this.textareaWidth) { this.hostElement.style.setProperty('--textarea-width', this.textareaWidth); this.hostElement.setAttribute('textarea-width', ''); } else { this.hostElement.style.removeProperty('--textarea-width'); this.hostElement.removeAttribute('textarea-width'); } }
-
Conflicting width styles: The
textareaWidthprop is still passed toTextareaElementand applied as an inlinewidthstyle on the<textarea>element inpackages/core/src/components/input/input.fc.tsx. This will override thewidth: 100%from the stylesheet and cause incorrect layout. You should remove thewidth: props.textareaWidthfrom thestyleobject inTextareaElementto allow the textarea to correctly fill its container.
nuke-ellington
left a comment
nuke-ellington
left a comment
There was a problem hiding this comment.
- Rows and width act like max-width/-height which they shouldn't. User should still be able to drag the textarea to be larger like the native HTML textarea handles it
- Please resolve Gemini review
- Please resolve Sonar issues
|
|
Will be fixed via #2293. |
4 participants
💡 What is the current behavior?
textareaRows property has no noticable effect on the component.
textareaWidth property relative values (e.g. 50%) will not work as expected
GitHub Issue Number: #1975 only textarea
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support