feat(textarea): adjusting min-height of the textarea for ionic theme#30957
feat(textarea): adjusting min-height of the textarea for ionic theme#30957rugoncalves wants to merge 33 commits intonextfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where textareas with the rows attribute set to 1 (or other values) would not properly respect the row count due to forced min-height values in the Ionic theme. The fix adjusts padding placement and removes fixed min-height constraints when the rows attribute is present.
Changes:
- Removed hardcoded
min-heightvalues from.textarea-size-*classes in the Ionic theme - Added
min-height: autooverride for textareas with therowsattribute - Moved padding from
.textarea-wrapper-innerto.native-textareawhenrowsis set (for proper scrolling behavior) - Added comprehensive e2e tests for the
rowsfunctionality
Reviewed changes
Copilot reviewed 4 out of 91 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
textarea.ionic.scss |
Removed min-height from size classes; added padding redistribution for rows attribute |
textarea.common.scss |
Added min-height: auto override for textareas with rows attribute |
test/rows/textarea.e2e.ts |
New comprehensive test suite for rows functionality |
test/rows/index.html |
New manual testing page for rows feature |
| Various snapshot files | Updated visual regression test snapshots |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@rugoncalves for future PRs, please make sure to link the original PR since it is helpful to review feedback left by the reviewers. I've already added the link. |
| * Rows attribute is only available in the Ionic theme. | ||
| * When set, it increases the container min-height to accommodate the number of rows. |
There was a problem hiding this comment.
Let's be consistent with the other tests:
| * Rows attribute is only available in the Ionic theme. | |
| * When set, it increases the container min-height to accommodate the number of rows. | |
| * This behavior does not vary across directions | |
| * This behavior only applies to the `ionic` theme. |
| */ | ||
| configs({ modes: ['ionic-md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('textarea: rows'), () => { | ||
| test('should respect rows attribute and set min-height', async ({ page }) => { |
There was a problem hiding this comment.
| test('should respect rows attribute and set min-height', async ({ page }) => { | |
| test('should respect rows attribute, async ({ page }) => { |
There was a problem hiding this comment.
We don't need to have examples of multiple rows example. Keep it simple:
- Default (no size) with the row of 3
- Small with the row of 3
- Medium with the row of 3
- Large with the row of 3
- Default (no size) with the row of 3 and autosize
There was a problem hiding this comment.
One of the things this task solves, as stated in the PR, is the height of the text area when rows is equal to 1 (it was equivalent to the height of rows=2). Given this, having tests that different row number, specially lower than the default 3, would be valuable.
But as the final word is yours, let me know, if I should proceed with removing the tests mentioned.
There was a problem hiding this comment.
I agree with Rúben we need to make sure we are testing rows=1 at the very least and probably a larger one too.
There was a problem hiding this comment.
Remove the label-placement attribute since the rows changes is not associated to the label based on the CSS.
There was a problem hiding this comment.
Property removed from the tests.
There was a problem hiding this comment.
Remove the helper-text since we don't need to extra indication of how many rows there are since it's being defined in the header.
There was a problem hiding this comment.
Property removed from the tests.
| :host([rows]:not([auto-grow])) .start-slot-wrapper, | ||
| :host([rows]:not([auto-grow])) .end-slot-wrapper { | ||
| @include globals.padding(var(--padding-top), 0, var(--padding-bottom), 0); | ||
| } |
There was a problem hiding this comment.
Why is this needed? There's no example on the test page or in the E2E tests. Add an example to verify that these styles are needed.
There was a problem hiding this comment.
I've added a test to verify the relevance of these styles: d460dcc
| await expect(container).toHaveScreenshot(screenshot(`textarea-rows-different-values`)); | ||
| }); | ||
|
|
||
| test('should respect rows attribute with fill outline and solid', async ({ page }) => { |
There was a problem hiding this comment.
Remove this test since the change is not dependent to the outline. Make sure to delete the old snapshots too.
| ); | ||
|
|
||
| const container = page.locator('#container'); | ||
| await expect(container).toHaveScreenshot(screenshot(`textarea-rows-different-sizes`)); |
There was a problem hiding this comment.
| await expect(container).toHaveScreenshot(screenshot(`textarea-rows-different-sizes`)); | |
| await expect(container).toHaveScreenshot(screenshot(`textarea-rows-sizes`)); |
Make sure to delete the old snapshots too.
There was a problem hiding this comment.
Depending on the decision made above, this should be or not removed.
| await expect(container).toHaveScreenshot(screenshot(`textarea-rows-different-sizes`)); | ||
| }); | ||
|
|
||
| test('should respect rows attribute with value content', async ({ page }) => { |
There was a problem hiding this comment.
Delete this test since it's the same as the first one. Make sure to delete the old snapshots too.
There was a problem hiding this comment.
Depending on the decision made above, this should be or not removed.
| ); | ||
|
|
||
| const textarea = page.locator('ion-textarea'); | ||
| await expect(textarea).toHaveScreenshot(screenshot(`textarea-rows-3-autogrow`)); |
There was a problem hiding this comment.
| await expect(textarea).toHaveScreenshot(screenshot(`textarea-rows-3-autogrow`)); | |
| await expect(textarea).toHaveScreenshot(screenshot(`textarea-rows-autogrow`)); |
Make sure to delete the old snapshots too.
There was a problem hiding this comment.
Depending on the decision made above, this should be or not removed.
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
Issue number: resolves internal
What is the current behavior?
heightof the textarea would not correspond to the rows number, when it the row number was set to1:What is the new behavior?
min-heightis now respecting the rows attribute:.textarea-size-*classes stopped forcing themin-height;Does this introduce a breaking change?
Other information
Original PR