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

fix(ng-maxLength): rerender mdMaxLength when ngModel is updated in the controller #8312

Closed
wants to merge 2 commits into from

Conversation

josefromsanjose
Copy link

@josefromsanjose josefromsanjose commented May 2, 2016

This issue comes up when sharing one form for several models. For example a list. If you were to click on one item that displays the form and it has a value for the scope variable being counted, it works fine. But if you change to a different list item that does not have a value, mdMaxLength keeps the count from the previous model, but the input is empty.

@josefromsanjose josefromsanjose changed the title updage mdMaxLength when ngModel is update in the controller: Update input.js rerender mdMaxLength when ngModel is updated in the controller: Update input.js May 2, 2016
@@ -524,7 +524,13 @@ function mdMaxlengthDirective($animate, $mdUtil) {
element.on('input keydown keyup', function() {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you could achieve the same without using a watcher by doing ngModelCtrl.$parsers.push(renderCharCount); here. Also it would be nice if you could add a unit test for this so it doesn't get broken in the future.

@josefromsanjose
Copy link
Author

@crisbeto that was my first approach but it did not work. The problem still persists.

@crisbeto
Copy link
Member

crisbeto commented May 4, 2016

Alright, you should still be able to reduce it to scope.$watch(attr.ngModel, renderCharCount). Also can you rename your commit and PR so they match the format of the other PRs?

@josefromsanjose josefromsanjose changed the title rerender mdMaxLength when ngModel is updated in the controller: Update input.js fix(ng-maxLength): rerender mdMaxLength when ngModel is updated in the controller May 6, 2016
@josefromsanjose
Copy link
Author

@crisbeto I renamed to PR but could not rename the commits. Moving forward I will make sure I check what commit titles look like. This is my first open source contribution.

@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label May 6, 2016
@devversion
Copy link
Member

devversion commented May 6, 2016

I'm sorry, that I created something like a second approach, but your PR wasn't referencing the issues properly. So there is a more generic / universal solution now.

See #8351

cc. @crisbeto

@crisbeto
Copy link
Member

crisbeto commented May 6, 2016

Yeah that definitely looks better.

@crisbeto crisbeto removed the needs: review This PR is waiting on review from the team label May 6, 2016
@crisbeto
Copy link
Member

crisbeto commented May 6, 2016

@josefromsanjose Thank you for the PR, but I'll have to close it, because @devversion's solution solves the problem in a better way, removes a bunch of event listeners and has unit tests.

@crisbeto crisbeto closed this May 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants