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

Support custom locale #105

Merged
merged 10 commits into from
Jul 30, 2017
Merged

Support custom locale #105

merged 10 commits into from
Jul 30, 2017

Conversation

fakenickels
Copy link
Contributor

In order to work now, the user should create a file like this and initialize it before all yup usage.

import { setLocale } from 'yup/lib/locale'

setLocale({
 email: { invalid: 'Não é válido' },
})

// then after...
import yup from 'yup'

I'm going to add tests and update README after you guys approve this!
Thanks for the great package btw. Let me know if any changes are necessary before merging.

Supports now the user passing her own locale which will get deep merged and the missing fields will default to the current one
src/locale.js Outdated
@@ -1,13 +1,19 @@
import { merge } from 'lodash/fp'
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can use Object.assign instead of a lodash module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for lodash.merge because it'll merge deeply, so the user can change just one entry and let the rest default for the current ones.

Copy link
Owner

Choose a reason for hiding this comment

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

you aren't doing any deep merging though, since it's merging each namespace individually. I'd prefer to maintain that as well since merge is not the tiniest part of lodash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what I wanted, to merge each namespace inside the locale object.
image

But I think you are right, I'm going to just make an or case instead of merging. (export let string = customLocale.string || {...}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, now I got what you meant haha
I'm going to make the adjustments soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@jquense
Copy link
Owner

jquense commented Jul 18, 2017

Thanks for taking the time to do this 👍

@fakenickels
Copy link
Contributor Author

Done @jquense

@guilhermedecampo
Copy link

In need of this.

README.md Outdated
@@ -138,6 +139,19 @@ schema.cast({
// => { name: 'jimmy', age: 24, createdOn: Date }
```

### Using a custom locale dictionary
```js
Copy link
Owner

Choose a reason for hiding this comment

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

can you flesh this out a bit more, explanation of the code, why you need to do this, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'll do it soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done @jquense

@blocknomad
Copy link

👍

README.md Outdated

// Now use Yup schemas AFTER you defined your custom dicionary
```
Why do you have to set the locales before you use Yup at all?
Copy link
Owner

Choose a reason for hiding this comment

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

I was think more about what this does generally, not why it needs be first. something like:

"Allows you to customize the default messages used by Yup, when no message is provided with a validation test."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh ok

@jquense jquense merged commit 5d9522a into jquense:master Jul 30, 2017
@jquense
Copy link
Owner

jquense commented Jul 30, 2017

thanks!

Tadwork pushed a commit to Tadwork/yup that referenced this pull request Apr 18, 2018
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