-
Notifications
You must be signed in to change notification settings - Fork 498
feat: create with vue-i18n #548
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start.
There should be at least one snapshot test to check that everything works.
Ideally, the should introduced a modified component template to use a translation that could be checked in a test.
.idea/vcs.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove all the .idea files that you added to this commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thnink this is the default value, so the line can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when used in TS, there are additional informations that should be added to have a proper typechecking:
https://vue-i18n.intlify.dev/guide/advanced/typescript.html
@cexbrayat
Thank you for review!
Ideally, the should introduced a modified component template to use a translation that could be checked in a test.
I've got it !
Is it okay to do minimal component testing first, and then plan to test all possible cases later? Of course, I want to keep responding continuously.
@cexbrayat
vuejs/create-vue-templates#4
I wrote snapshot test case. I'd appreciate it if you could check this for me.
@Procrustes5 Sorry, I should have explained: to add a snapshot test, you don't have to open a PR on create-vue-templates, you have to update the snaphot.mjs script in this PR. create-vue-templates is then automatically updated when we release a new version of create-vue.
@cexbrayat
Thank you!
I've changed snapshot. plz check this!
scripts/snapshot.mjs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be removed from here because it's going to generate too many snapshots
template/tsconfig/vue-i18n/i18n.d.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this file should be necessary? With TypeScript, you need to do something like:
import { createI18n } from 'vue-i18n' import enUS from './locales/en-US.json' // Type-define 'en-US' as the master schema for the resource type MessageSchema = typeof enUS const i18n = createI18n<[MessageSchema], 'en-US'>({ locale: 'en-US', messages: { 'en-US': enUS } })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And i think this file can be removed as well?
@cexbrayat
Thank you for review!
You mean template/tsconfig/vue-i18n is unnecessary, is it correct?
I have changed, please check it one more time!
@Procrustes5
As I was pointing out, when using vue-i18n, the generated src/i18n/index.ts file should look like this I think:
import { createI18n } from 'vue-i18n' import enUS from './locales/en-US.json' type MessageSchema = typeof enUS const i18n = createI18n<[MessageSchema], 'en-US'>({ locale: 'en-US', messages: { 'en-US': enUS } })
Also, as a second step, the generated HelloWorld component could show the usage of t() (for example https://github.com/vuejs/create-vue/blob/main/template/code/default/src/components/HelloWorld.vue could have You’ve successfully created a project with translated). And the generated unit test could check this to ensure the addition of vue-i18n works well.
Oh and I forgot something: the PR should also update ci.yml to actually test the vue-i18n flag.
We don't want to test with all combinations, but maybe you could do the same that was done for the devtools flag.
...5/create-vue into feat/create-with-i18n
@cexbrayat
Thank you so much for the easy explanation. I've modified it, so please check it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I'm just wondering if the components which are the same in the typescript-default template could not be removed to avoid duplication. Other than that, looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file necessary? If it doesn't change from the typescript-default versions, then I think it should not be duplicated if possible (same for the other components).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cexbrayat
Oh, I'd not understood this architecture yet. Thank you
modified now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@cexbrayat
It has been left here for a long time in a state where I can't proceed with anything. Is there anything I can do?
Description
issue : #545
Added vue-i18n to the option. I made it with the directory structure suggested in the above issue.