Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Feature: custom Message Element & update parameter for setErrorHTML() & setSuccessHTML() #183

Merged
merged 1 commit into from
Jan 11, 2016

Conversation

hung-doan
Copy link
Contributor

New feature:

  • Add new function: $validationProvider.addMsgElement()
  • Add new function: $validationProvider.getMsgElement()

Update feature:
From

$validationProvider.setErrorHTML(function(msg){
});

$validationProvider.setSuccessHTML(function(msg){
});

To

$validationProvider.setErrorHTML(function(msg, element, attrs){
});

$validationProvider.setSuccessHTML(function(msg, element, attrs){
});

@hueitan
Copy link
Owner

hueitan commented Jan 2, 2016

👍 This looks good @hungdoan2

See the comment.

};

}]);
```
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mind giving more description about add/getMsgElement ?

This is difficult to understand,

  1. Show the default message element
  2. Show the example why we use addMsgElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Owner

Choose a reason for hiding this comment

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

Also, I would like to add a line

Please make sure that you understand the usage before using add/getMsgElement

@hueitan
Copy link
Owner

hueitan commented Jan 4, 2016

It will be perfect if you can squash your commit into more readable message. 🍺

@hueitan
Copy link
Owner

hueitan commented Jan 5, 2016

@hungdoan2 I'm ready to merge the commit after you finish.

@hung-doan hung-doan force-pushed the feature.setMessageElement branch from 07e5348 to bd0d85c Compare January 5, 2016 09:53
@hung-doan
Copy link
Contributor Author

@huei90 already squashed all commits & update document.
About document I think It will be easier to follow If there is a navigation bar in document page , which will show all the API/Feature need, like this: http://angular-ui.github.io/ui-router/site/#/api/ui.router

@hueitan
Copy link
Owner

hueitan commented Jan 5, 2016

Please rebase the master lastest code.

navigation bar

Yes, this is the ticket I'm working on.

@hung-doan hung-doan force-pushed the feature.setMessageElement branch 2 times, most recently from ade91d2 to f39dd5f Compare January 5, 2016 11:52
@hung-doan
Copy link
Contributor Author

@huei90 already rebased :)


if (element.attr('no-validation-message')) {
messageElem.css('display', 'none');
} else if ($validationProvider.showSuccessMessage && messageToShow) {
messageElem.html('').append($compile($validationProvider.getSuccessHTML(messageToShow))(scope));
messageElem.html($validationProvider.getSuccessHTML(messageToShow, element, attrs));
Copy link
Owner

Choose a reason for hiding this comment

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

@hungdoan2 This should be rebased too. the $compile.

@hung-doan hung-doan force-pushed the feature.setMessageElement branch from f39dd5f to b06bfd3 Compare January 5, 2016 12:57
@hung-doan
Copy link
Contributor Author

@huei90 hope this commit work 💪

@hueitan
Copy link
Owner

hueitan commented Jan 5, 2016

@hungdoan2 Yes this works perfectly 🍻

@hueitan
Copy link
Owner

hueitan commented Jan 5, 2016

@lvarayut I'm going to merge this commit, please review.

@@ -362,3 +362,43 @@ angular.module('yourApp', ['validation'])
};
}]);
```


### **Setup Message Element in config phase**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain more in plain English? For example, "You could use this addMsgElement to ... It reduces your effort of manually putting `message-id" so on and so forth.

@lvarayut
Copy link
Collaborator

lvarayut commented Jan 5, 2016

@hungdoan2 Would you mind to provide some unit tests covering your new functions?

@hueitan
Copy link
Owner

hueitan commented Jan 6, 2016

@hungdoan2 I can do the unit tests job if you want me to do it.

@hung-doan
Copy link
Contributor Author

@huei90 Thanks you, I'm glad to hear that, Would u might helping me with these test cases! . Actually I got a trouble with grunt test: It always show Can not load "PhantomJS", it is not registered!. even I installed all phantom packages. I'm checking it now.

@hueitan
Copy link
Owner

hueitan commented Jan 6, 2016

@hungdoan2 try npm install, else uninstall -g phantomjs and try install again.


@hungdoan2 This is a big feature for us, so it takes time to complete all the tasks before merging.

@hung-doan
Copy link
Contributor Author

@huei90 ok, I will complete my task in this week 💪 . and I'm sorry to rush you^^

@hueitan
Copy link
Owner

hueitan commented Jan 8, 2016

@hungdoan2 No worry 😄

@hung-doan hung-doan force-pushed the feature.setMessageElement branch from c0dcd13 to 65da3e7 Compare January 9, 2016 19:31
@hung-doan hung-doan force-pushed the feature.setMessageElement branch from 65da3e7 to 932aea2 Compare January 9, 2016 22:03

})); //END it

}); //END describe
Copy link
Owner

Choose a reason for hiding this comment

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

This is fine 😄 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@hueitan
Copy link
Owner

hueitan commented Jan 11, 2016

This commit is almost done, I'm going to merge it now.

hueitan pushed a commit that referenced this pull request Jan 11, 2016
New Feature: custom Message Element & update parameter for setErrorHTML() & setSuccessHTML()
@hueitan hueitan merged commit 5c05445 into hueitan:master Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants