Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

Procrustes5
Copy link

Description

issue :#545

Added vue-i18n to the option. I made it with the directory structure suggested in the above issue.

Copy link
Member

@cexbrayatcexbrayat left a 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.

Procrustes5 reacted with heart emoji
.idea/vcs.xml Outdated
<mapping directory="" vcs="Git" />
<mapping directory="$PROJECT_DIR$/playground" vcs="Git" />
</component>
</project>
Copy link
Member

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?

Procrustes5 reacted with thumbs up emoji

export default createI18n({
legacy: false,
globalInjection: true,
Copy link
Member

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

Procrustes5 reacted with thumbs up emoji
locale: 'en',
fallbackLocale: 'en',
messages
})
Copy link
Member

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

Procrustes5 reacted with thumbs up emoji
@Procrustes5
Copy link
Author

@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.

@Procrustes5
Copy link
Author

@cexbrayat
vuejs/create-vue-templates#4
I wrote snapshot test case. I'd appreciate it if you could check this for me.

@cexbrayat
Copy link
Member

@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 thesnaphot.mjs script in this PR.create-vue-templates is then automatically updated when we release a new version of create-vue.

Procrustes5 reacted with thumbs up emoji

@Procrustes5
Copy link
Author

@cexbrayat
Thank you!
I've changed snapshot. plz check this!

@@ -15,7 +15,8 @@ const featureFlags = [
'vitest',
'cypress',
'playwright',
'nightwatch'
'nightwatch',
'vue-i18n'
Copy link
Member

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

Procrustes5 reacted with thumbs up emoji
export interface DefineLocaleMessage {
hello: string
}
}
Copy link
Member

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'importenUSfrom'./locales/en-US.json'// Type-define 'en-US' as the master schema for the resourcetypeMessageSchema=typeofenUSconsti18n=createI18n<[MessageSchema],'en-US'>({locale:'en-US',messages:{'en-US':enUS}})

Procrustes5 reacted with thumbs up emoji
"types": ["i18n"]
},
"include": ["src/template/config/vue-i18n/**/*", "src/template/config/vue-i18n/**/*.json"]
}
Copy link
Member

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?

Procrustes5 reacted with thumbs up emoji
@Procrustes5
Copy link
Author

@cexbrayat
Thank you for review!
You meantemplate/tsconfig/vue-i18n is unnecessary, is it correct?
I have changed, please check it one more time!

@cexbrayat
Copy link
Member

@Procrustes5
As I was pointing out, when usingvue-i18n, the generatedsrc/i18n/index.ts file should look like this I think:

import{createI18n}from'vue-i18n'importenUSfrom'./locales/en-US.json'typeMessageSchema=typeofenUSconsti18n=createI18n<[MessageSchema],'en-US'>({locale:'en-US',messages:{'en-US':enUS}})

Also, as a second step, the generated HelloWorld component could show the usage oft() (for examplehttps://github.com/vuejs/create-vue/blob/main/template/code/default/src/components/HelloWorld.vue could haveYou’ve successfully created a project with translated). And the generated unit test could check this to ensure the addition ofvue-i18n works well.

Procrustes5 reacted with thumbs up emoji

@cexbrayat
Copy link
Member

cexbrayat commentedAug 14, 2024•
edited
Loading

Oh and I forgot something: the PR should also updateci.yml to actually test thevue-i18n flag.
We don't want to test with all combinations, but maybe you could do the same that was done for thedevtools flag.

Procrustes5 reacted with thumbs up emoji

@Procrustes5
Copy link
Author

@cexbrayat
Thank you so much for the easy explanation. I've modified it, so please check it out!

Copy link
Member

@cexbrayatcexbrayat left a 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 thetypescript-default template could not be removed to avoid duplication. Other than that, looks good to me.

Procrustes5 reacted with thumbs up emoji
flex-wrap: wrap;
}
}
</style>
Copy link
Member

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 thetypescript-default versions, then I think it should not be duplicated if possible (same for the other components).

Copy link
Author

@Procrustes5Procrustes5Aug 17, 2024•
edited
Loading

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!

<div class="greetings">
<h1 class="green">{{ msg }}</h1>
<h3>
{{ t('greetings.message') }}
Copy link
Member

Choose a reason for hiding this comment

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

👍

Procrustes5 reacted with laugh emoji
@Procrustes5
Copy link
Author

@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?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@cexbrayatcexbrayatAwaiting requested review from cexbrayat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@Procrustes5@cexbrayat
[8]ページ先頭
©2009-2025 Movatter.jp