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

Able to compile validation output message? (template validation message) #96

Closed
jamhall opened this issue Mar 17, 2015 · 22 comments
Closed
Assignees

Comments

@jamhall
Copy link

jamhall commented Mar 17, 2015

Hello,

Great module!

I would like to translate the default messages for example, I have this:

  app.config(['$validationProvider', function ($validationProvider) {
    var expression = {
      required: function (value) {
        return !!value;
      }
    };

    var defaultMsg = {
      required: {
        error: 'generic.form.validations.required'
      }
    };

    $validationProvider.setExpression(expression).setDefaultMsg(defaultMsg);

    $validationProvider.setErrorHTML(function (msg) {
      return "<span class=\"label label-danger\">{{ msg | translate }}</span>";
    });
  }]);

Please note that generic.form.validations.required is a translation key.

It would be nice if this module could interpolate the error html so I have the translated message.

Any ideas to solve this?

Thanks!

@hueitan
Copy link
Owner

hueitan commented May 11, 2015

Hi @jamhall I prefer not to do this in the validation method. We should handle this on the javascript function, not in angular way.

I'm going to close this, open it if needed.

@hueitan hueitan closed this as completed May 11, 2015
@jamhall
Copy link
Author

jamhall commented May 11, 2015

Hello, could you be a bit more explicit please? Got a work around for
others? There is currently no other way to do it! My commit doesn't break
anything. Thanks.
On 11 May 2015 06:05, "Huei Tan" notifications@github.com wrote:

Hi @jamhall https://github.com/jamhall I prefer not to do this in the
validation method. We should handle this on the javascript function, not in
angular way.

I'm going to close this, open it if needed.


Reply to this email directly or view it on GitHub
#96 (comment)
.

@jamhall
Copy link
Author

jamhall commented May 11, 2015

Users should have the ability to use a custom template. You allow custom
html but not a parsed template, how could I use the translate function, or
any other template function if the template is not parsed?
On 11 May 2015 06:05, "Huei Tan" notifications@github.com wrote:

Hi @jamhall https://github.com/jamhall I prefer not to do this in the
validation method. We should handle this on the javascript function, not in
angular way.

I'm going to close this, open it if needed.


Reply to this email directly or view it on GitHub
#96 (comment)
.

@hueitan
Copy link
Owner

hueitan commented May 11, 2015

Hi @jamhall

In your example, you are using filter to translate your message.

You can do the same using javascript function

 $validationProvider.setErrorHTML(function (msg) {
   return "<span class=\"label label-danger\">" + translate(msg) + "</span>";
 });

// ...

function translate (msg) {
    // get translation data dictionary
    // ... blablabal translate
    return translate_msg;
}

@hueitan hueitan reopened this May 11, 2015
@hueitan
Copy link
Owner

hueitan commented May 11, 2015

Let me think about this.

@jamhall
Copy link
Author

jamhall commented May 11, 2015

Your example would not work because the template is not parsed :(
On 11 May 2015 10:08, "Huei Tan" notifications@github.com wrote:

Hi @jamhall https://github.com/jamhall

In your example, you are using filter to translate your message.

You can do the same using javascript function

$validationProvider.setErrorHTML(function (msg) {
return "<span class="label label-danger">" + translate(msg) + "";
});
// ...
function translate (msg) {
// get translation data dictionary
// ... blablabal translate
return translate_msg;
}


Reply to this email directly or view it on GitHub
#96 (comment)
.

@hueitan
Copy link
Owner

hueitan commented May 11, 2015

What's the meaning of "the template is not parsed" ?

In the source code, It's just output a html code only.

messageElem.html($validationProvider.getSuccessHTML(messageToShow));

I suppose that output validation message should be simple, therefore I didn't involved compile here.

@jamhall
Copy link
Author

jamhall commented May 11, 2015

But that's the point. If you cannot use a custom template, then you cannot use any template functions.

You have put this function:

function translate (msg) {
    // get translation data dictionary
    // ... blablabal translate
    return translate_msg;
}

This would not work. You cannot call a modules function in the app.run or app.config functions. You only have access to provider functions.

Most angular modules allow a template to be parsed. I don't see why this is a problem for you?

@jamhall
Copy link
Author

jamhall commented May 11, 2015

And by parsing I mean something like this:

 <span class="label label-danger"> {{ "Hello World" | uppercase }} </span>

Why, with your current module am I unable to call template functions?

@hueitan
Copy link
Owner

hueitan commented May 12, 2015

Hi @jamhall Thanks for reaching this question.

Seems in your case you need those "template" like validation message, I think it's needed to compile if we provide template validation.

But for now, validation message is simple and plain, maybe next version we can have the message in template for more customisation 😄 .

Add this as enhancement.

@hueitan hueitan changed the title Translate default messages Able to compile validation output message? (template validation message) May 12, 2015
@Nazanin1369 Nazanin1369 self-assigned this Dec 23, 2015
@Nazanin1369
Copy link
Collaborator

@jamhall You do not need to compile the HTML for translation, you can call it programmatically. Just put up a commit for this.
After merge you are able to setup translation as following:

 return '<p class="validation-valid">' + this.$injector.get('$translate').instant(msg) + '</p>';

for using filters you can inject the $filter and use any filter you like as:

  var $filter = this.$injector.get('$filter')
  return  '<p class="validation-valid">' + $filter('uppercase')(msg) + '</p>';

@hueitan
Copy link
Owner

hueitan commented Dec 27, 2015

@Nazanin1369 nice fix.
Would you mind read this PR?
#96

Let's consider both solutions.

@jamhall
Copy link
Author

jamhall commented Dec 29, 2015

@Nazanin1369 - I feel that {{ msg | translate }} is a lot cleaner than this.$injector.get('$translate').instant(msg)

I don't like to put code like that into my template strings.

I honestly don't see what the problem is, and why this is being over-complicated, the solution that I have offered is clean and is only a couple of lines of code and it also doesn't depend on any other dependencies.

I've looked at your commit, you're injecting the translate module, why? Not everyone will be using the translation module... it should be optional. If I were to use your commit, I wouldn't be able to update the angular-validation module with any changes. I'd like to stick to main source tree. Also, it's unnecessary, if the template string is compilled, then I can just call the translate (or any other) function without injecting another dependency. Thank you though.

Have you looked at my PR?

The point is, people should be able to use a validation template like a normal template without resorting to hacks.

Thank you!

Apologies for the delay in replying....currently on holiday.

@hueitan
Copy link
Owner

hueitan commented Dec 29, 2015

Hi @jamhall

Sorry for nudging you back to this topic and I also want to apologize to you that your suggestion is not that bad.

{{ msg | translate }} is a lot cleaner than this.$injector.get('$translate').instant(msg)

Totally Agree.

It's also useful for other things such as msg | lowercase or msg | uppercase for example.

Usually in angularjs, we want to do a lot of compilation, even though the directive itself, so the compilation for the output html template is needed.

you're injecting the translate module

Yes, we haven't merged that commit yet because we have some concern, here's the discussion board #178

Have you looked at my PR?

Since I haven't closed this issue because I though this may have some serious discussions in some days and we are seriously thinking about this now. Hope you can understand and continually giving us more suggestions and directions.


@Nazanin1369

Sorry for putting you in this situation and I know we have the discussion about $injector before which we had some agreements. Let's rethinking about this, hopefully, we can sketch up the final answer.

@jamhall
Copy link
Author

jamhall commented Dec 29, 2015

@huei90 - thanks for your prompt reply

Since I haven't closed this issue because I though this may have some serious discussions in some days and we are seriously thinking about this now. Hope you can understand and continually giving us more suggestions and directions.

I still don't understand why this needs to be continually discussed. What's the problem? It's a simple and transparent change. It doesn't break anything. It doesn't change anything for existing users and it adds needed functionality. Why discuss this further? I appreciate you want to open and transparent....but this functionality was asked for (and the code was provided) nine months ago...

@hueitan
Copy link
Owner

hueitan commented Dec 29, 2015

@jamhall You are right, this change doesn't break anything and I also hope this can be merged soon.

As you can see, this library is growing bigger now, 
and I'm not the only one who developing - we are in the collaboration group. 
That's mean we should work together to make this library going far. 
Before merging, I would like to wait for other collaborators to notice this change and giving their opinions.

Sorry again @jamhall, we take every commits/issues seriously so that I can't make an immediate decision by my own.

There are something we should do before merging

  1. Documentation for the compilation for html template
  2. Making some test cases (if you see the latest commit, we have coverage now)

Please give us some time and you will see the merging soon 🍻

@jamhall
Copy link
Author

jamhall commented Dec 29, 2015

@huei90 - Ok, excellent. thank you for clearing that up. I look forward to seeing the changes in the near future. 🍻

@hueitan
Copy link
Owner

hueitan commented Dec 29, 2015

You will see this soon.

You have my word 😄

@Nazanin1369
Copy link
Collaborator

@jamhall You are compiling againts the $rootScope which triggers a digest cycle on clients app from the root to last child scope which is a performance hit.

@hueitan
Copy link
Owner

hueitan commented Dec 31, 2015

@Nazanin1369 I'm going to merge this feature compile since we have no better solution to do this.
We can have more discussion in the future until we got a cleaner and low-cost implementation for the client app.

@hueitan
Copy link
Owner

hueitan commented Jan 4, 2016

New pr created here.
#184

@hueitan hueitan closed this as completed Jan 4, 2016
@hueitan
Copy link
Owner

hueitan commented Jan 4, 2016

@jamhall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants