d-kostov-dev / ng2-mdf-validation-messages

Angular 2 Model Driven Forms Validation Messages
MIT License
11 stars 8 forks source link

Change to be able to override a single error message #10

Closed SanderLedegen closed 7 years ago

SanderLedegen commented 7 years ago

I noticed that when I define a defaultErrorMessages object in the config, I have to specify all possible error messages with a custom message. If I don't, this causes an error because the error message obviously doesn't exist and thus placeholders can't be replaced further on in the code.

As I didn't want to specify all error messages and just wanted to override a single message, I created this pull request that made this possible 😊

d-kostov-dev commented 7 years ago

Are you sure that this is indeed the problem?

I am currently running the demo app with this setup:

Ng2MDFValidationMessagesModule.globalConfig({ defaultErrorMessages: { required: 'Default Custom Required Message' } }),

And it's working fine.

Also your fix looks like the same thing, that is 2 lines above:

if (customConfig) {
    this.config = Object.assign({}, defaultConfig, customConfig);
}

this.messageProvider = new MessageProvider(this.config.defaultErrorMessages);

Can you share an example where it's not working.

Thanks!

P.S. I also found a setup bug in the demo-app I'll update it now so you can check it.

SanderLedegen commented 7 years ago

Hmm, I can indeed confirm the demo app is working the way I would expect. Unfortunately the project where I was having the problem was more of a temporarily something. The external services it requires are no longer available so I can't properly set it up and test it again 😕 .

My configuration was as follows (similar to the config in the demo app):

Ng2MDFValidationMessagesModule.globalConfig({
    class: 'error-validation alert alert-danger',
    defaultErrorMessages: {
        required: 'This field cannot be left empty',
    },
})

The problem was that only the required message was known in the final config object because Object.assign "merges" defaultErrorMessages as a whole and does not copy properties individually to the target object. It just checks the presence of defaultErrorMessages and copies it to the target object when it finds it. This pushed me in the direction of specifying either every message or nothing, just as I said in my original pull request. I created a quick fiddle to show you exactly what I mean right here. That's why I Object.assigned defaultErrorMessages itself instead of the whole object, in my proposed solution.

But whatever, seems I was mistaken for some reason. Sorry for raising this "problem" 😊

d-kostov-dev commented 7 years ago

Ok. I see it!

Now I'm wondering why the F is it working in the demo app! :) Will check it in Monday.

Thanks.

SanderLedegen commented 7 years ago

After a quick glance, it seems the demo app is suddenly not working entirely correct for me (anymore?). I re-cloned the repo and started playing with the demo app. Most fields show an appropriate error when I enter incorrect values but as soon as I type any value in the age field, I get the exact same error I was having in my application. Did I miss this in my previous test or is something else going on? It's kind of strange and I'm planning to take a look as well 😃

Another thing I noticed was that the password fields are not checked for equality even though they should according to the code. I thought I'd share these two points with you which might help us wrap our heads around these issues.

d-kostov-dev commented 7 years ago

Sorry for the delay. Merged. Will release NPM version later.

SanderLedegen commented 7 years ago

Awesome, thanks! 👍