DatePicker, DateRangePickerをdayjsでやりとりするようにwrap #1080

maktak1995 · 2022-10-31T09:20:16Z

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
Localize added component

netlify · 2022-10-31T09:20:24Z

✅ Deploy Preview for ingred-ui ready!

Name	Link
🔨 Latest commit	`95dbf97`
🔍 Latest deploy log	https://app.netlify.com/sites/ingred-ui/deploys/636e134e2b70de000a64b12e
😎 Deploy Preview	https://deploy-preview-1080--ingred-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

…ngred-ui into replaceMomentToDayjs

maktak1995 · 2022-11-07T09:54:02Z

src/components/DatePicker/DatePicker.tsx

+      if (!renderMonthTextProps || !dayjsize) return;
+      return renderMonthTextProps(dayjsize);
+    };
+


SingleDatePickerはmomentなので、外側でdayjsでlocaleを設定しても影響しない。
外からdayjsのlocale設定をコンポーネントへ持ち込むことはできなかったので locale と(必要なら) weekdayShort を渡すやり方にしてみた

takurinton · 2022-11-10T08:14:03Z

漏れがありそうです
https://github.com/voyagegroup/ingred-ui/blob/master/src/components/LocaleProvider/LocaleProvider.stories.tsx#L173

maktak1995 · 2022-11-10T08:35:49Z

@takurinton 漏れてたとこ直しました〜

takurinton

どれも細かい点ですがコメントしました。
大まかなやり方は良さそう、:thx:

takurinton · 2022-11-10T07:52:16Z

package.json

@@ -26,6 +26,7 @@
  },
  "dependencies": {
    "@popperjs/core": "^2.4.0",
+    "dayjs": "^1.11.6",


Suggested change

"dayjs": "^1.11.6",

"dayjs": "1.11.6",

takurinton · 2022-11-10T07:53:39Z

src/components/DatePicker/DatePicker.tsx

@@ -61,7 +97,13 @@ const DatePicker = React.forwardRef<HTMLDivElement, DatePickerProps>(
              </Spacer>
            </Styled.NavNext>
          }
+          // eslint-disable-next-line react/jsx-handler-names


Suggested change

// eslint-disable-next-line react/jsx-handler-names

takurinton · 2022-11-10T08:13:45Z

.vscode/settings.json

+  },
+  "editor.defaultFormatter": "dbaeumer.vscode-eslint"


この差分はミスってるだけですかね？

ミスですね

takurinton · 2022-11-10T08:33:39Z

src/components/DatePicker/DatePicker.tsx

+      date,
+      error = false,
+      onDateChange,
+      renderMonthText: renderMonthTextProps,


renderMonthTextProps だと複数を指しているように見えてしまうので単数にするのが良さそうなのと、XXXProps だと名前が関数っぽくないので何か違う名前がつけられると良さそう。
あとは、これ今の構成だとそもそも別名つけなくても良さそうに見えるのですがどうでしょう。

takurinton · 2022-11-10T08:40:20Z

src/components/DatePicker/DatePicker.tsx

+    const handleRenderMonthText = (month: moment.Moment) => {
+      const dayjsize = momentToDayjs(month);
+      if (!renderMonthTextProps || !dayjsize) return;
+      return renderMonthTextProps(dayjsize);
+    };


renderMonthTextProps の有無に関わらず dayjsize が作られてしまうので renderMonthTextProps の存在確認と dayjsize の null 確認は別の条件分岐で書いた方がよさそうです。

nits
linter 案件ではありますが（自分の惰性が働き strict-boolean-expressions を有効にしていないのが原因です...。）、論理否定を使った falsy 判定はあまり行儀が良くない気がします。

そこまで気にすることでもないので falsy に関しては任せます

…replaceMomentToDayjs

maktak1995 · 2022-11-11T08:03:14Z

@takurinton 修正いれたんで確認お願いします〜

takurinton · 2022-11-11T08:09:28Z

package.json

+    "react": "17.0.0 || 18.0.0",
+    "react-dom": "17.0.0 || 18.0.0",


peerDependencies は変えないほうがいいです。
これだと 17.0.0 or 18.0.0 の決めうちになってしまいます。

takurinton · 2022-11-11T08:20:45Z

src/components/DatePicker/DatePicker.tsx

+      onDateChange(dayjsize);
+    };
+    const handleRenderMonthText = (month: moment.Moment) => {
+      if (renderMonthText == undefined || null) return;


これだと

renderMonthText が undefined だった場合

早期 return される

renderMonthText が undefined ではなかった場合

if (null) が評価されて falsy になるので次に処理が進む

となるはずで、一見挙動としては正しいように見えますが自分が nits レビューで指摘した falsy をなくしたいは達成できてないように見えます。

また、推測ですが renderMonthText == undefined || renderMonthText == null を書きたかった感じですかね？
JavaScript の式で null や undefined を扱う際は厳密な比較だと null と undefined は区別する、曖昧な比較だと区別しないというようになっているので単純に

if (renderMonthText == undefined) return;

でいいように感じます。

takurinton · 2022-11-11T08:25:58Z

src/components/DatePicker/DatePicker.tsx

+    const handleRenderMonthText = (month: moment.Moment) => {
+      if (renderMonthText == undefined || null) return;
+      const dayjsize = momentToDayjs(month);
+      if (dayjsize == null) return;


momentToDayjs の戻り値は Dayjs | null なので厳密に比較しとくのが吉です。

nyago

2735e9f

maktak1995 self-assigned this Oct 31, 2022

maktak1995 and others added 5 commits October 31, 2022 18:20

Update settings.json

112bf4f

Update settings.json

ad4c49d

nyasu

e88d160

Merge branch 'replaceMomentToDayjs' of ssh://github.com/voyagegroup/i…

ebebd98

…ngred-ui into replaceMomentToDayjs

nyago

c38ca39

maktak1995 commented Nov 7, 2022

View reviewed changes

maktak1995 added 3 commits November 8, 2022 16:55

dayjsのlocaleDataを渡すスタイルにしてみる

8021b0c

nya

6c5f737

nyasu

4beb7e4

maktak1995 marked this pull request as ready for review November 10, 2022 07:47

maktak1995 requested a review from takurinton November 10, 2022 07:47

変え漏れを直す

2f503db

takurinton reviewed Nov 10, 2022

View reviewed changes

maktak1995 added 7 commits November 11, 2022 16:46

nyago

f2bdb75

nyanya

a6c606b

Merge branch 'master' of ssh://github.com/voyagegroup/ingred-ui into …

eb4ce2d

…replaceMomentToDayjs

ぱけじなおす

3a5e650

DatePickerなおす

7ba31c9

DateRangePickerなおす

fa67975

やーん

2e1b445

maktak1995 requested a review from takurinton November 11, 2022 08:02

takurinton reviewed Nov 11, 2022

View reviewed changes

maktak1995 added 2 commits November 11, 2022 18:08

ぴあ

11c9b96

nya

95dbf97

maktak1995 requested a review from takurinton November 11, 2022 09:18

takurinton approved these changes Nov 11, 2022

View reviewed changes

maktak1995 merged commit e6c42f3 into master Nov 11, 2022

maktak1995 deleted the replaceMomentToDayjs branch November 11, 2022 09:25

maktak1995 mentioned this pull request Feb 16, 2023

DatePicker/DateRangePicker のリニューアル #975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DatePicker, DateRangePickerをdayjsでやりとりするようにwrap #1080

DatePicker, DateRangePickerをdayjsでやりとりするようにwrap #1080

maktak1995 commented Oct 31, 2022

netlify bot commented Oct 31, 2022 •

edited

Loading

maktak1995 Nov 7, 2022

takurinton commented Nov 10, 2022

maktak1995 commented Nov 10, 2022

takurinton left a comment

takurinton Nov 10, 2022

takurinton Nov 10, 2022

takurinton Nov 10, 2022

maktak1995 Nov 11, 2022

takurinton Nov 10, 2022

takurinton Nov 10, 2022

takurinton Nov 10, 2022

takurinton Nov 10, 2022

maktak1995 commented Nov 11, 2022

takurinton Nov 11, 2022

maktak1995 Nov 11, 2022

takurinton Nov 11, 2022

takurinton Nov 11, 2022

		"react": "17.0.0 \|\| 18.0.0",
		"react-dom": "17.0.0 \|\| 18.0.0",

DatePicker, DateRangePickerをdayjsでやりとりするようにwrap #1080

DatePicker, DateRangePickerをdayjsでやりとりするようにwrap #1080

Conversation

maktak1995 commented Oct 31, 2022

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

netlify bot commented Oct 31, 2022 • edited Loading

✅ Deploy Preview for ingred-ui ready!

Choose a reason for hiding this comment

takurinton commented Nov 10, 2022

maktak1995 commented Nov 10, 2022

takurinton 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

maktak1995 commented Nov 11, 2022

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

netlify bot commented Oct 31, 2022 •

edited

Loading