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

fix: Localize the duration in sponsor section #136

Merged
merged 6 commits into from
May 12, 2024

Conversation

KannoStanfoot
Copy link
Contributor

@KannoStanfoot KannoStanfoot commented May 10, 2024

Issue

https://github.com/vuejs-jp/vuefes-2024-backside/issues/151

Details of the changes

  • スポンサーセクションの期間のローカライズが正しくされていない問題を修正した
  • Storybookのi18n対応を行なった

Screenshots

Before

ja en
Vue_Fes_Japan_2024 Vue_Fes_Japan_2024

After

ja en
Vue_Fes_Japan_2024 Vue_Fes_Japan_2024

Storybook

確認用URL Chromatic
date___DatePeriod_-_i18n_⋅_Storybook

Copy link

netlify bot commented May 10, 2024

Deploy Preview for vuefes-2024 ready!

Name Link
🔨 Latest commit 165747c
🔍 Latest deploy log https://app.netlify.com/sites/vuefes-2024/deploys/6640b082620b660008d4ba9d
😎 Deploy Preview https://deploy-preview-136--vuefes-2024.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@KannoStanfoot KannoStanfoot changed the title fix: Localize the duration in sponsor section [WIP]fix: Localize the duration in sponsor section May 10, 2024
@KannoStanfoot
Copy link
Contributor Author

KannoStanfoot commented May 10, 2024

Done.
WIP: Chromaticのディレクトリ指定でエラーが発生しているので修正する。
Cursor_と__WIP_fix__Localize_the_duration_in_sponsor_section_·_vuejs-jp_vuefes-2024_acd767c

@KannoStanfoot KannoStanfoot changed the title [WIP]fix: Localize the duration in sponsor section fix: Localize the duration in sponsor section May 10, 2024
apps/web/app/lang/ja.json Outdated Show resolved Hide resolved
:start="{
prefixYear: $t('sponsor.prefixYear'),
date: $t('sponsor.start_date'),
dayOfWeek: $t('sponsor.day_of_week.monday'),
Copy link
Member

Choose a reason for hiding this comment

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

$t には key が見つからなかった場合に引数にデフォルトメッセージをしていすれば、そのデフォルトメッセージで解決するので、それを利用すると en 側のリソースに空文字を指定する必要がない気がする。
https://vue-i18n.intlify.dev/api/injection.html#t-key-defaultmsg

Suggested change
dayOfWeek: $t('sponsor.day_of_week.monday'),
dayOfWeek: $t('sponsor.day_of_week.monday', ''),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kazupon
デフォルトメッセージの指定、空文字だと挙動としてkeyを返却するようです。

dayOfWeek: $t('sponsor.day_of_week.monday', ''),
期待結果: ''
結果:'sponsor.day_of_week.monday'


dayOfWeek: $t('sponsor.day_of_week.monday', 'x'),
期待結果: 'x'
結果:'x'

Copy link
Member

Choose a reason for hiding this comment

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

あ、ホントですね。すいません、これ vue-i18n のデフォルトメッセージのエッジケースなバグっぽいですね...
あとは、

  1. useI18n が露出する locale を使って en のときは空文字を与える
  2. useI18n が露出する te を使って sponsor.day_of_week.monday key が vue-i18n で有効になっているlocale ないときは、空文字を与える

SponsorPageSection.vue 内部で workaround 的に対応する感じですかね...
あとは現状のまま行くか。

Copy link
Contributor Author

@KannoStanfoot KannoStanfoot May 11, 2024

Choose a reason for hiding this comment

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

2 の方法で空文字を与えるよう修正しました。

const { t, te } = useI18n()
const { locale } = useLocaleCurrent()
/**
* Get translation or return empty string
* @param key - translation key
* @returns translation or empty string
*/
function getTranslationOrDefault(key: string): string {
return te(key, locale.value) ? t(key) : ''
}
const periodStart = {
prefixYear: t('prefix_year'),
date: t('start_date'),
dayOfWeek: getTranslationOrDefault('day_of_week.monday'),
}
const periodEnd = {
suffixYear: t('suffix_year'),
date: t('end_date'),
dayOfWeek: getTranslationOrDefault('day_of_week.thursday'),
}

Copy link
Member

Choose a reason for hiding this comment

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

あ、ホントですね。すいません、これ vue-i18n のデフォルトメッセージのエッジケースなバグっぽいですね...

ちなみに、これは vue-i18n v10 で直しておきました。
intlify/vue-i18n#1849

(nuxt i18n にはまだ取り込めていない。)

apps/web/app/lang/en.json Outdated Show resolved Hide resolved
@KannoStanfoot
Copy link
Contributor Author

📝 lang/ のリソース定義、スネークケースに統一する

Comment on lines +10 to +20

const { t, te } = useI18n()
const { locale } = useLocaleCurrent()
/**
* Get translation or return empty string
* @param key - translation key
* @returns translation or empty string
*/
function getTranslationOrDefault(key: string): string {
return te(key, locale.value) ? t(key) : ''
}
Copy link
Contributor Author

@KannoStanfoot KannoStanfoot May 11, 2024

Choose a reason for hiding this comment

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

@jiyuujin @toshick @ryuhei373
[Ask]
この部分、tsファイルで管理した方が汎用性あると思いつつ
本プロジェクトでどのディレクトリで管理すべきか判断つかなかったのでコンポーネントに直接実装しています。

外出しする場合、どのディレクトリで管理するのが良いでしょうか?

Copy link
Collaborator

Choose a reason for hiding this comment

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

外出しする場合、どのディレクトリで管理するのが良いでしょうか?

apps/web/app/composables で良いかと思います

Copy link
Member

Choose a reason for hiding this comment

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

getTranslationOrDefault ではなく composable なので、useTranslation にしておくとよいかと思います。

Copy link
Collaborator

@jiyuujin jiyuujin left a comment

Choose a reason for hiding this comment

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

翌朝10:00よりスポンサー公募第二弾が開始する予定なので、
いったんマージしますね mm

後ほどリファクタリングを進めていただければ(急ぎませんので mm

@jiyuujin jiyuujin merged commit 02dde23 into main May 12, 2024
12 of 13 checks passed
@jiyuujin jiyuujin deleted the feature/sponsor-section-localization branch May 12, 2024 12:13
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.

3 participants