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

fix(input): add support for dynamically loaded ngMessage directives. #8387

Closed

Conversation

devversion
Copy link
Member

@devversion devversion commented May 9, 2016

  • Currently ngMessage directives inside of an input-container, which are loaded / transcluded dynamically, didn't get initialized properly.
    The animation was not working. As well as the auto-hide didn't work properly.

cc. @topherfangio

Fixes #7477. Fixes #7596. Fixes #6810. Fixes #7823

@devversion devversion added the needs: review This PR is waiting on review from the team label May 9, 2016
* Currently ngMessage directives inside of an input-container, which are loaded / transcluded dynamically, didn't get initialized properly.
  The animation was not working. As well as the auto-hide didn't work properly.

Fixes angular#7477. Fixes angular#7596. Fixes angular#6810. Fixes angular#7823
@devversion
Copy link
Member Author

@topherfangio Added the tests as discussed :)

expect(ngMessage).toHaveClass('md-input-message-animation');
}));

it('should set the animation class on a transcluded ngMessage', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if we could do a different/shorter test here.

Based on the issues this is supposed to fix, if we put it inside an autocomplete, or use the ng-messages-include directive, it would fail correct (I mean, without this PR)?

Is there a reason to do it as a document fragment test rather than one of the standard transclusion methods listed in the issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, as I commented on the test, using the document fragment is testing the new added code in the input component.

Using a tranclusion here and testing it, would be more complicated.

I will give it a try with the ng-messages-include.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, if that's the case, then go ahead and add the ng-messages-include as a separate test if you can 😄

Thanks for the clarification!

@devversion devversion deleted the fix/input-message-fragment branch May 19, 2016 20:05
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants