Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(textarea): fix textarea grow #5757

Closed

Conversation

devversion
Copy link
Member

Fixes Textarea Input

Fixes #5751

@ThomasBurleson ThomasBurleson added needs: review This PR is waiting on review from the team and removed needs: review This PR is waiting on review from the team labels Nov 24, 2015
@ThomasBurleson ThomasBurleson modified the milestones: 1.0-rc7, 1.01 Nov 24, 2015
@devversion
Copy link
Member Author

@ThomasBurleson @EladBezalel
I created another commit (not squashed yet - by intention - ability to revert), which contains min-rows (Minimum Rows), rows (Fixed Rows) and unspecified (starting at 1).

Can you please review that commit and give me a suggestion about adding the min-rows attribute. Should we use another attribute name?

@morriq
Copy link

morriq commented Nov 27, 2015

Working well on my website. 👍

@devversion
Copy link
Member Author

Update

  • Added complicated tests for min-rows, rows and unspecified rows attribute
  • Fixed Minimum Rows if text content is exactly at minimum

PS: @EladBezalel Can you please take a look 😄 + (not yet squashed)

@@ -0,0 +1,22 @@
<div layout="column" ng-cloak class="md-inline-form">

<md-content layout-padding>
Copy link
Member

Choose a reason for hiding this comment

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

layout-padding is making the text areas look weird, wrap them with divs or make a class

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied that from the Basic Demo, or is this a difference because of the other inputs?

Copy link
Contributor

Choose a reason for hiding this comment

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

The CSS for layout-padding changed recently. We are aware of some issues with it, but the quick workaround is to wrap the content in a <div>.

@devversion devversion force-pushed the fix/textarea-default-height branch from d5dbd9c to a57b716 Compare November 28, 2015 23:53
@devversion
Copy link
Member Author

The most of your suggestions are now ready. @EladBezalel

@EladBezalel EladBezalel added the needs: team discussion This issue requires a decision from the team before moving forward. label Dec 4, 2015
@morriq
Copy link

morriq commented Dec 30, 2015

Seems there is problem with this PR.

sorry for no codepen or something. I don't know how to include changed library to sourcecode.

It's removing default value if exists.

<md-input-container ng-init="test = '123';">
<textarea ng-model="test"></textarea>
</md-input-container>


element.on('scroll', onScroll);
}

// Trigger the input event to compute style
element.triggerHandler('input');
Copy link

Choose a reason for hiding this comment

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

this line causing problems(It's removing default value if exists.)

@devversion devversion force-pushed the fix/textarea-default-height branch from 7630498 to d2ec04b Compare December 30, 2015 10:23
@devversion
Copy link
Member Author

All problems are now solved @morriq
Thanks for keeping an eye on that :)

PS: Ignore Travis CI -> It's currently always failing due an error in websockets/ws

@devversion devversion force-pushed the fix/textarea-default-height branch from d2ec04b to 101b1bc Compare January 5, 2016 14:51
@ThomasBurleson ThomasBurleson modified the milestones: 1.0.1, 1.0.2 Jan 5, 2016
@ThomasBurleson ThomasBurleson removed this from the 1.0.1 milestone Jan 5, 2016

node.style.height = calcHeight + "px";
} else if (contentHeight) {
node.style.height = contentHeight + 'px';
Copy link
Member

Choose a reason for hiding this comment

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

i'd expect a variable to save the height into and applying node.style.height = only once for more readability

@EladBezalel
Copy link
Member

Hey @devversion, because we're past 1.0 we try our best not to introduce breaking changes.
We should make this PR without any breaking API, if you will need my help ping me.

It's also fixing #5919 ?

@devversion
Copy link
Member Author

@EladBezalel Just thought a bit about the breaking change. I don't think it such a huge breaking change, it's just adding the attribute min-rows to allow growing rows.

Little Explanation:

  • rows="5" - Fixed 5 Rows
  • min-rows="5" - Minimum 5 Rows (can grow)

I think it's not a big change, and I think it also needed to provide this new attribute to allow the users to decide how the <textarea> should behave. Let me know what you think? And what are you thinking @ThomasBurleson?

PS: And yes - this should fix #5919 too, and please take a look at the new <textarea> demos from the PR 😄

@EladBezalel
Copy link
Member

@ThomasBurleson assigned to you not because it's ready, please review.

@alexthewilde
Copy link

+1

@EladBezalel
Copy link
Member

As this is not the complete and desired behavior of textarea, im closing this one,
@crisbeto can you please open an issue about our new documented textarea and note there our concerns and wanted behavior?

@devversion devversion deleted the fix/textarea-default-height branch April 19, 2016 19:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: review This PR is waiting on review from the team needs: team discussion This issue requires a decision from the team before moving forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants