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

feat: create with vue-i18n #548

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

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

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

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


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

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

@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

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'
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
  }
})

"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
Copy link
Author

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

@cexbrayat
Copy link
Member

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

@cexbrayat
Copy link
Member

cexbrayat commented Aug 14, 2024

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.

@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

@cexbrayat cexbrayat 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 the typescript-default template could not be removed to avoid duplication. Other than that, looks good to me.

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

Copy link
Author

@Procrustes5 Procrustes5 Aug 17, 2024

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.

👍

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.

2 participants