Feature: Locale Provider #434

kohashi · 2021-08-16T01:38:32Z

Ref #312

Check List (If️ you added new component in this PR)

Export the component in src/components/index.ts
Add example to .storybook/documents/Information/Samples/Samples.stories.tsx

netlify · 2021-08-16T01:38:35Z

✔️ Deploy Preview for ingred-ui ready!

🔨 Explore the source changes: 215ac11

🔍 Inspect the deploy log: https://app.netlify.com/sites/ingred-ui/deploys/612f1007e5e5dc00076088b8

😎 Browse the preview: https://deploy-preview-434--ingred-ui.netlify.app

kohashi · 2021-08-16T01:43:16Z

TODO:

<LocaleProvider locale={jaJP}> とかで注入できるようにする
jaJP, zhCN, enUS まで定義
story上で言語選択可に

kohashi · 2021-08-21T19:12:27Z

レビュー可能になった気がするのでどなたかお願いします。

youchann · 2021-08-23T01:16:10Z

今日のお昼あけあたりにみます！

youchann

とりあえずこんなところでしょうか。
基本相談っぽい感じになりましたが、説明が足りない場合はどんどん聞いちゃってください！

youchann · 2021-08-23T01:26:12Z

src/components/LocaleProvider/styled.ts

@@ -0,0 +1,3 @@
+import styled from "styled-components";
+
+export const Container = styled.div``;


yarn generateしていただいたと思うのですが、不要なら消しちゃう方針で大丈夫です:+1:

youchann · 2021-08-23T01:27:24Z

src/constants/locale.ts

+      defaultProps: { activeText: "开", inActiveText: "关" },
+    },
+    ConfirmModal: {
+      defaultProps: { confirmText: "确认", cancelText: "取消" },


取消だけ読めた..w

youchann · 2021-08-23T01:34:27Z

src/utils/useLocaleProps.ts

+  const defaultProps = locale?.components?.[name].defaultProps;
+  let propName;
+
+  for (propName in defaultProps) {


自分はいつも下記のような書き方をするのですが、letを用いる/用いないでなにか差が出たりしますか..?(純粋に気になっただけです)

Suggested change

for (propName in defaultProps) {

for (const propName in defaultProps) {

hoistingも一度しか起きないしブロックスコープでconstを再定義するよりも早くなる余地があるからです。(実務上の件数ではおそらく計測不能なほどの高速化ですが)
…と思ったけど試したことがないので

// 準備 const PROPS_COUNT = 100000; const props = {}; for (let i = 0; i < PROPS_COUNT; i++) { props[i.toString()] = i.toString(); } // constのやつ console.time(); for (const outKey in props) { // doSomething } console.timeEnd(); // default: 9.113037109375 ms // letのやつ console.time(); let outKey for (outKey in props) { // doSomething } console.timeEnd(); // default: 6.7509765625 ms

どうでしょう！
初回のテストではやはり letのほうが高速 でした！

・・・というのが初回のテストだったんですが、初回がチャンピオンデータだったようで
再読込するたびにほぼ同じになったり、constのほうが高速になったり、逆だったり、ばらつきが激しいです。

デバッグコンソールの実行では？読み込み時では？といろいろ試したもののばらつきは変わらず。
https://codepen.io/kohashi/pen/GRERMVw とかで書いてみたけど

ちなみに node ( v15.13.0 ) でもためしてみました。こちらは安定していて

default: 14.598ms // constのほう
default: 7.452ms // letのほう

let のほうが早い！しかもダブルスコアで！！！

しかし、ふと古いnodeだとどうだろう？と思い v8.17.0 (特に理由はなくなんとなく v8 latest)で試すと

undefined: 8.979ms // constのほう
undefined: 8.476ms // letのほう

・・・ 🤔

まぁ console.time の仕様が違うくらい昔なので何らかの実装が違うようでした。

というわけで「実務上差がないレベルで let 使ったほうが早いと思ったんだけど、計測してみたらなんかブロックスコープの生成か何かが実装によってバラバラっぽいため速度もバラバラ。node v15では安定して早いっぽいが」

そんな感じでした。

youchann · 2021-08-23T02:38:38Z

src/utils/useLocaleProps.ts

+    if (output[propName] === undefined) {
+      output[propName] = defaultProps[propName];
+    }


type HogeProps = { hoge: { text: "hogehoge"; someValue: true; } };

こんな感じのネストした値には対応できていない気がするのですがどうでしょう...?

https://github.com/TehShrike/deepmerge

このnpmで説明できるかは微妙ですが、オブジェクト同士をマージするような形式(undefinedの場合のみ上書き)みたいなやり方が良いかなぁ今の所思っております！

propsのマージ、 shallow merge, or deep merge の話ですよね。
ここはshallowでいいんじゃないかって思ってました。

const ToggleButton: React.FunctionComponent<ToggleButtonProps> = ({ activeText = "ON", // propsを受けてデフォ値を代入する、というのと、l10nはよく似ている hoge = { text: "hogehoge", someValue: true} // こういうのと感覚的には同じだから、shallowがよりよいんじゃないかなって }) => { // ....

速度的な観点もありはするものの、基本的にはl10nする側の定義は大きくないので、deepmergeが必要ならやってもよいとは思いますー。
(MUIはシャローマージにしているものの、だから使い勝手悪くてあまり定義がないのか、そもそも基本UIフレームワークにl10nが必要な文字ってそんなに無いのでは？ということなのかの意図は判然としない…後者の気がしますが)

そもそも基本UIフレームワークにl10nが必要な文字ってそんなに無いのでは？

なるほど、MUIもshallowなのですね。
deepmergeへ途中から変更するのもそこまで難しくないので、現段階ではshallowでいく、というので良い気がしました！

ここ書いてて思ったのですが、<MultipleFilter />も対応が必要そうですね。

youchann · 2021-08-23T02:42:13Z

src/utils/useLocaleProps.ts

+ * @param params
+ * @returns localized Props.
+ */
+export function useLocaleProps<T>(params: {


この"カスタムフック"に該当するものはhooks/に集めているのでそちらに移動しても良さそうです:+1:

hoooksのほうがベター。なるほどです。移しますー

youchann · 2021-08-23T02:44:50Z

src/constants/locale.ts

+  };
+}
+
+export const jaJP: Localization = {


このあたりの変数、ライブラリの利用者からも使えるようにする方が良い気がしておりまして。

src/index.tsxあたりからexportすると良さそうです！

exportしましたー

youchann · 2021-08-23T02:46:44Z

src/components/LocaleProvider/LocaleProvider.stories.tsx

+  };
+
+  return (
+    <LocaleProvider locale={locales[localeOption.value]}>


【相談】
storybookで書けているので良いかなぁと思いつつも、READMEの方も<LocaleProvider />を用いるようにしたら親切かなと思うのですがどうですかね...?

どうでしょうね。。
READMEには最小限だけ書いて、別途docsをしっかり書くのが流行りというか主流ですが、今はstorybookがDocs代わりになってるならいらないのでは？とも思ったりします。

「あんまりl10n周りを READMEの最初にガッツリ書かないよなー」くらいの温度感です。

kohashi · 2021-08-24T03:12:56Z

一旦残りを整理

<MultipleFilter /> もl10n対応
- エラーメッセージとからしい
localesをexportして利用者がカスタムできる
- 利用者が新たなlocale (e.g. koKR とか) を定義できる
- 利用者が既存locale定義の一部修正ができる (e.g. jaJP でカタカナオン・オフを英語に戻すなど)

kohashi · 2021-08-27T03:16:34Z

|| " で検索すると、 <MultipleFilter /> 以外にもいくつかありそう。
props のデフォ値としてではなく、tsxのインラインで <Hoge hogeText={propsValue || "default message"} みたいな記述。

EditFilterCard, FilterCard, かな

kohashi · 2021-08-27T03:41:40Z

EditFilterCard

formPlaceholder
inputErrorText
formErrorText
editButtonTitle
sectionTitle, conditionTitle

FilterCard

formPlaceholder
inputErrorText
formErrorText
applyButtonTitle
sectionTitle, conditionTitle

違いは editButtonTitleかapplyButtonTitleかの違いかな。
それと MultipleFilter の placeholder

…o impl/locale

kohashi · 2021-08-30T11:38:09Z

すごい大変だった…。

@youchann お元気になったらレビューお願いします。議論になりそうなとこはコードにコメントしておきます。

kohashi · 2021-08-30T11:41:20Z

src/constants/locale.ts

+        title: "把文件拖入,  同样支持点击上传。",
+      },
+    },
+    ItemEmpty: { defaultProps: { title: "未找到。" } },


翻訳する量が増えちゃったのと、中国語チェックしてくれるヒトが忙しくなっちゃったので、中国語版は半分くらいしかない…。

kohashi · 2021-08-30T11:42:30Z

src/components/ItemEmpty/ItemEmpty.tsx

-);
+const ItemEmpty: React.FunctionComponent<ItemEmptyProps> = (inProps) => {
+  const props = useLocaleProps({ props: inProps, name: "ItemEmpty" });
+  const { title = "Not found.", subtitle, emptyImage, imageWidth = 80 } = props;


ItemEmpty の NotFound も useLocaleProps の対象にしました。

kohashi · 2021-08-30T11:49:27Z

src/components/MultipleFilter/MultipleFilter.tsx

@@ -212,7 +218,7 @@ const MultipleFilter: React.FunctionComponent<MultipleFilterProps> = ({
              ref={setInputElement}
              readOnly
              type="text"
-              placeholder={placeholder ?? "Add a new filter"}
+              placeholder={placeholder}


こういう ?? Nullish Coalescing だったところを、 props = 'Default Value' に移す & useLocaleProps で上書きみたいなことを何箇所かでやっています。

?? " と || " で検索したのでl10n必要なとこは一旦埋めた気がするけど、まだ大きくあれば一旦マージして別PRにて対応したく…

kohashi · 2021-08-30T11:52:55Z

src/components/MultipleFilter/internal/FilterCard/FilterCard.tsx

@@ -187,7 +195,7 @@ export const FilterCard: React.FunctionComponent<Props> = ({
        {selectedFilter && (
          <div>
            <Typography weight="bold" size="lg">
-              {filter?.conditionTitle || "Condition"}
+              {filter?.conditionTitle || conditionTitle}


ここが一番迷ったんですが、　conditionTitle, sectionTitle は props に移設しました。

props に移設しないやり方としては、

FilterCard: { defaultProps: { applyButtonTitle: "適用", inputErrorText: "入力してください", formPlaceholder: "検索", sectionTitle: "セクション", conditionTitle: "条件", }, }, // ↓ FilterCard: { defaultProps: { applyButtonTitle: "適用", inputErrorText: "入力してください", formPlaceholder: "検索", }, texts: { sectionTitle: "セクション", conditionTitle: "条件", }, },

こんな風にすることも考えたんですが、一旦propsに統一したほうが利用者側でカスタマイズが容易で良いかな、と。

youchann

LGTMです！

youchann · 2021-08-31T00:17:05Z

src/components/LocaleProvider/__tests__/__snapshots__/LocaleProvider.test.tsx.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`LocaleProvider component testing LocaleProvider 1`] = `<DocumentFragment />`;


思えばProviderって実態の無いUIなのでスナップショットも不要な気がしますね！

確かにそうだなって感じたので、スナップショット除外して生成出来るかだけのテスト(toBeTruthy)にしました!

kohashi · 2021-09-01T05:32:36Z

セルフマージしちゃっていいのかな

kohashi · 2021-09-01T05:59:19Z

reviewed になったら権限あるひとはセルフマージしてるな。よーししちゃうぞー

wip: impl locale provider

2925fc9

kohashi added 9 commits August 17, 2021 12:54

fix: LocaleProivder wrapped

b75963e

feat: useLocaleProps function

257b054

add: locale selection on storybook

fee6046

localization: ConfirmModal

a804263

localization: FileUploader

f379f39

localization: ItemEmpty

7fc900a

chore: export

59f80d8

storybook:

4b8ed68

lint:

0d542e6

kohashi marked this pull request as ready for review August 21, 2021 19:11

kohashi requested a review from a team as a code owner August 21, 2021 19:11

kohashi requested review from youchann and removed request for a team August 21, 2021 19:11

storybook: styling

858fc12

youchann assigned kohashi Aug 23, 2021

youchann reviewed Aug 23, 2021

View reviewed changes

kohashi changed the title ~~wip: impl locale provider~~ Feature: Locale Provider Aug 23, 2021

kohashi added 2 commits August 24, 2021 10:10

rm: remove unused styled.ts

0565bb2

mv: useLocaleProps

bab1990

kohashi added 3 commits August 27, 2021 11:12

rm: no clear difference in execution speed, let/const.

b4ff65f

add: export locales

a0982f6

storybook: add custom locale definition

30d2803

kohashi added 6 commits August 30, 2021 00:03

fix: extract jsx inline strings to props

89d3f44

feat: add l10n to filter components

9f49cf5

test: update snapshot

a64b092

Merge branch 'master' of https://github.com/voyagegroup/ingred-ui int…

344aa06

…o impl/locale

l10n: add "not found" l10n

e871c1e

rm: unused

6db730a

kohashi commented Aug 30, 2021

View reviewed changes

youchann approved these changes Aug 31, 2021

View reviewed changes

fix: remove snapshot from LocaleProvider

215ac11

kohashi merged commit d403ac9 into master Sep 1, 2021

kohashi deleted the impl/locale branch September 1, 2021 05:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Locale Provider #434

Feature: Locale Provider #434

kohashi commented Aug 16, 2021 •

edited

Loading

netlify bot commented Aug 16, 2021 •

edited

Loading

kohashi commented Aug 16, 2021 •

edited

Loading

kohashi commented Aug 21, 2021

youchann commented Aug 23, 2021

youchann left a comment

youchann Aug 23, 2021

youchann Aug 23, 2021

youchann Aug 23, 2021

kohashi Aug 24, 2021

youchann Aug 23, 2021

youchann Aug 23, 2021

kohashi Aug 24, 2021

youchann Aug 24, 2021

youchann Aug 23, 2021

kohashi Aug 24, 2021

youchann Aug 23, 2021

kohashi Aug 30, 2021

youchann Aug 23, 2021

kohashi Aug 24, 2021

kohashi commented Aug 24, 2021 •

edited

Loading

kohashi commented Aug 27, 2021

kohashi commented Aug 27, 2021

kohashi commented Aug 30, 2021

kohashi Aug 30, 2021

kohashi Aug 30, 2021

kohashi Aug 30, 2021

kohashi Aug 30, 2021

youchann left a comment

youchann Aug 31, 2021

kohashi Sep 1, 2021

kohashi commented Sep 1, 2021

kohashi commented Sep 1, 2021

		@@ -0,0 +1,3 @@
		import styled from "styled-components";

		export const Container = styled.div``;

	for (propName in defaultProps) {
	for (const propName in defaultProps) {

		@@ -0,0 +1,3 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`LocaleProvider component testing LocaleProvider 1`] = `<DocumentFragment />`;

Feature: Locale Provider #434

Feature: Locale Provider #434

Conversation

kohashi commented Aug 16, 2021 • edited Loading

Check List (If️ you added new component in this PR)

netlify bot commented Aug 16, 2021 • edited Loading

kohashi commented Aug 16, 2021 • edited Loading

kohashi commented Aug 21, 2021

youchann commented Aug 23, 2021

youchann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohashi commented Aug 24, 2021 • edited Loading

kohashi commented Aug 27, 2021

kohashi commented Aug 27, 2021

kohashi commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youchann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohashi commented Sep 1, 2021

kohashi commented Sep 1, 2021

kohashi commented Aug 16, 2021 •

edited

Loading

netlify bot commented Aug 16, 2021 •

edited

Loading

kohashi commented Aug 16, 2021 •

edited

Loading

kohashi commented Aug 24, 2021 •

edited

Loading