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

fix(input): remove unnecessary warnings when ng-messages not provided #11352

Merged
merged 1 commit into from
Jul 26, 2018
Merged

fix(input): remove unnecessary warnings when ng-messages not provided #11352

merged 1 commit into from
Jul 26, 2018

Conversation

marosoft
Copy link
Contributor

There is no requirement to include the messages in case of an error.
Highlighting the field can be enough.

Fixes #10461

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

At the moment warning are displayed in the console when ng-messages are not specified for the input field.

Issue Number:
Relates to #10461
Relates to #9468

What is the new behavior?

The warnings are not generated as it is not required to include the error messages.
The original fix for #9468 is still in place. The other changes introduced together with that fix were removed.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jun 29, 2018
@Splaktar Splaktar self-requested a review July 5, 2018 20:55
@Splaktar Splaktar self-assigned this Jul 5, 2018
@Splaktar Splaktar added this to the 1.1.11 milestone Jul 5, 2018
@Splaktar Splaktar added type: bug hotlist: animations P1: urgent Urgent issues that should be addressed in the next minor or patch release. labels Jul 5, 2018
Splaktar
Splaktar suggested changes Jul 5, 2018
View reviewed changes
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Thank you for investigating this and submitting a PR! I just had a couple comments related to performance.

src/components/input/input.js Outdated
$log.warn('mdInput messages show animation called on invalid messages element: ', element);
done();
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand removing the warning, but this seems to also be removing the done(); call and the early return. I think that we still want those things for performance and correctness. We don't need to trigger the animations if there are no messages to animate in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, thanks. I will implement those changes.

As the animators array is empty it should not cause any performance issues. However for the correctness it makes sense to handle it here rather than depend on the behaviour of the called all function.

I will fix it as soon as possible.

src/components/input/input.js Outdated
$log.warn('mdInput messages hide animation called on invalid messages element: ', element);
done();
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. We don't need to animate the messages out if there are no messages.

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jul 5, 2018
Splaktar
Splaktar suggested changes Jul 9, 2018
View reviewed changes
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Thank you for making those updates. Can you please remove those two extra whitespaces and squash your commits?

src/components/input/input.js Outdated
@@ -1007,12 +1005,11 @@ function ngMessageAnimation($$AnimateRunner, $animateCss, $mdUtil, $log) {
function showInputMessages(element, done) {
var animators = [], animator;
var messages = getMessagesElement(element);
var children = messages.children();
var children = messages.children();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove the added whitespace at the end of this line?

src/components/input/input.js Outdated
@@ -1027,12 +1024,11 @@ function showInputMessages(element, done) {
function hideInputMessages(element, done) {
var animators = [], animator;
var messages = getMessagesElement(element);
var children = messages.children();
var children = messages.children();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove the added whitespace at the end of this line?

@marosoft
fix(input): remove unnecessary warnings when ng-messages not provided
f7d812e 
There is no requirement to include the messages in case of an error.
Highlighting the field can be enough.

Fixes angular#10461
@marosoft marosoft force-pushed the wip/input-messages-warning-fix-branch branch from 3fcd61f to f7d812e Compare July 11, 2018 18:36
@marosoft
Copy link
Contributor Author

@Splaktar just wanted to highlight that I squashed the commits

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: squash commits labels Jul 21, 2018
Splaktar
Splaktar approved these changes Jul 21, 2018
View reviewed changes
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba merged commit d48c5b8 into angular:master Jul 26, 2018
Splaktar pushed a commit that referenced this pull request Jul 31, 2018
@marosoft @Splaktar
fix(input): remove unnecessary warnings when ng-messages not provided (…
ccf6329 
…#11352)

There is no requirement to include the messages in case of an error.
Highlighting the field can be enough.

Fixes #10461
Splaktar pushed a commit that referenced this pull request Aug 2, 2018
@marosoft @Splaktar
fix(input): remove unnecessary warnings when ng-messages not provided (…
687d0a8 
…#11352)

There is no requirement to include the messages in case of an error.
Highlighting the field can be enough.

Fixes #10461
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Splaktar Splaktar Splaktar approved these changes

Assignees

@Splaktar Splaktar

@mmalerba mmalerba

Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ hotlist: animations P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Milestone
1.1.11
Development

Successfully merging this pull request may close these issues.

input: ng-messages animation errors
4 participants
@marosoft @Splaktar @googlebot @mmalerba