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

Allow getMessage to also pass objects to custom defaultMessageCreator. #46

Merged
merged 5 commits into from
Jan 4, 2017

Conversation

Zagitta
Copy link
Contributor

@Zagitta Zagitta commented Jan 2, 2017

So this is a small feature request to allow objects to be passed to defaultMessageCreator that would help our use case quite significantly.
To keep a long story short we're internationalizing our validation messages with react-intl by reimplementing the validators we need and passing a custom defaultMessageCreator to createValidator that returns an object instead of a string.
However when trying to internationalize the field parameter passed to defaultMessageCreator we run into an issue where only strings are accepted but the internationalization API requires an object consisting of id and defaultMessage.

This PR fixes that by allowing config.field to be an object while hopefully maintaining backwards compatibility.

Let me know if there's anything blocking this from being merged.

Best regards, Simon.

@jfairbank
Copy link
Owner

Hi @Zagitta! This seems like a reasonable change. I regret not baking i18n into revalidate already. That could be a useful future feature.

Would you mind adding a unit test for passing in an object to your PR? Fixing the failing typecheck might be more tricky. I don't care to fix that after merging if you're not comfortable with Flow.

Thanks for the PR!

@codecov-io
Copy link

Current coverage is 100% (diff: 100%)

Merging #46 into master will not change coverage

@@           master   #46   diff @@
===================================
  Files          29    29          
  Lines         208   209     +1   
  Methods        64    64          
  Messages        0     0          
  Branches       54    54          
===================================
+ Hits          208   209     +1   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update 8c43024...9cef4e4

@Zagitta
Copy link
Contributor Author

Zagitta commented Jan 4, 2017

Alright I've added a test as requested and tried my best to satisfy flow although to be perfectly honest I have no prior experience with it and don't reeeeally know what I'm doing so feel free to improve it 😄

Thanks for a neat little library by the way! It fits nicely together with redux-form 👍

@jfairbank jfairbank merged commit 9a5734c into jfairbank:master Jan 4, 2017
@jfairbank
Copy link
Owner

No worries. I'll tweak flow as need. Thanks for the changes and the PR!

@jfairbank
Copy link
Owner

@Zagitta this should be available in v1.1.0 now. Thanks again!

@lauterry
Copy link

Thanks @Zagitta and @jfairbank !!

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.

4 participants