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

Conversation

topherfangio
Copy link
Contributor

When using ng-messages inside of an input container, the height
of the messages will change when appearing, which pushes other
elements out of the way. This was a regression from the previous
behavior which allowed space for a one-line message before pushing
other content out of the way.

This was caused by some changes to the icons inside of an input.

Fix by finding an alternate method to address the icon issue.

Fixes #7072.

@topherfangio topherfangio added the needs: review This PR is waiting on review from the team label Feb 12, 2016
@topherfangio topherfangio added this to the 1.0.6 milestone Feb 12, 2016
@topherfangio
Copy link
Contributor Author

@EladBezalel Can you please review my changes when you get a moment? Everything looks good in the icons demo, but I wanted to double-check with you.

@topherfangio topherfangio added needs: work and removed needs: review This PR is waiting on review from the team labels Feb 12, 2016
@topherfangio
Copy link
Contributor Author

@EladBezalel NVM...just noticed an alignment issue with the icons that I didn't see before :/

@topherfangio topherfangio added needs: review This PR is waiting on review from the team and removed needs: work labels Feb 12, 2016
@@ -65,7 +65,15 @@ angular.module('material.components.input', [
*/
function mdInputContainerDirective($mdTheming, $parse) {

var INPUT_TAGS = ['INPUT', 'TEXTAREA', 'MD-SELECT'];
var INPUT_TAGS = ['INPUT', 'TEXTAREA', 'SELECT', 'MD-SELECT'];
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be in a const?

@EladBezalel
Copy link
Member

Nice improvement @topherfangio LGTM.
Checking locally

@EladBezalel EladBezalel added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Feb 12, 2016
When using `ng-messages` inside of an input container, the height
of the messages will change when appearing, which pushes other
elements out of the way. This was a regression from the previous
behavior which allowed space for a one-line message before pushing
other content out of the way.

This was caused by some changes to the icons inside of an input.

Fix by finding an alternate method to address the icon issue.

Fixes angular#7072.
@EladBezalel EladBezalel removed the needs: manual testing This issue or PR needs to have some manual testing and verification done label Feb 12, 2016

var LEFT_SELECTORS = INPUT_TAGS.reduce(function(selectors, isel) {
return selectors.concat(['md-icon ~ ' + isel, '.md-icon ~ ' + isel]);
}, []).join(",");
Copy link
Member

Choose a reason for hiding this comment

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

You can use .toString() instead of .join(',') they're equivalent on arrays

@topherfangio
Copy link
Contributor Author

@EladBezalel Just wanted to touch base, is this ready for merge, or does it still need manual testing? :-)

@EladBezalel
Copy link
Member

i've tested it manually and it's working good, what about the .toString?

@topherfangio
Copy link
Contributor Author

@EladBezalel I never knew that toString() would automatically add the commas, however, .join() is more clear in my opinion, and it's a character shorter, so unless there is a compelling reason to use .toString(), I would prefer to stick with join.

@EladBezalel EladBezalel added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Feb 15, 2016
@EladBezalel
Copy link
Member

BTW .join(',') is equals to .join()

@ThomasBurleson
Copy link
Contributor

@topherfangio - 👍

@topherfangio topherfangio deleted the fix-input-icons-7072 branch April 18, 2016 21:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
- Breaking Change pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants