Feat week with time selector component #1439

takurinton · 2023-09-27T09:06:47Z

No description provided.

changeset-bot · 2023-09-27T09:06:50Z

🦋 Changeset detected

Latest commit: 7184d5b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
ingred-ui	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2023-09-27T09:06:51Z

✅ Deploy Preview for ingred-ui ready!

Name	Link
🔨 Latest commit	`7184d5b`
🔍 Latest deploy log	https://app.netlify.com/sites/ingred-ui/deploys/6515179dd4c15100083eeacf
😎 Deploy Preview	https://deploy-preview-1439--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 configuration.

takurinton · 2023-09-27T09:16:39Z

プロダクト側では全選択・全解除みたいな機能作ったけど、そこまではやらない（onChange あるしプロダクト側で頑張って！）

takurinton · 2023-09-27T09:35:37Z

src/components/WeekTime/WeekTime.tsx

+import { convertTargetSettingToHex, getTargetSetting } from "./utils";
+
+export type WeekTimeProps = {
+  weekTime: string;


引数、42桁の16進数が入ってくることを型レベルで制御したいけど、重すぎて動かなくなるので一旦 string で進める

ちゃんと動くかわからないけど、こんな感じ...??

type HexDigit = | "0" | "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9" | "a" | "b" | "c" | "d" | "e" | "f"; type Join<T extends string[], S extends string> = T extends [] ? "" : T extends [infer F, ...infer R] ? F extends string ? `${F & string}${Join<Extract<R, string[]>, S>}` : never : never; type Repeat< S extends string, N extends number, T extends string[] = [], > = T["length"] extends N ? Join<T, S> : Repeat<S, N, [S, ...T]>; type WeekTime = Repeat<HexDigit, 42>;

ChatGPTに聞いたら同じような感じに

type HexDigit = '0' | '1' | '2' | '3' | '4' | '5' | '6' | '7' | '8' | '9' | 'a' | 'b' | 'c' | 'd' | 'e' | 'f'; type Join<T extends string[], S extends string = ''> = T extends [] ? S : T extends [infer F, ...infer R] ? Join<R, `${S}${F & string}`> : never; type Repeat<T extends string, N extends number, R extends string[] = []> = R['length'] extends N ? Join<R> : Repeat<T, N, [T, ...R]>; type Hex42 = Repeat<HexDigit, 42>;

もしくはシンプルにテンプレートリテラルを使うとかですかね？

type HexDigit = '0' | '1' | '2' | '3' | '4' | '5' | '6' | '7' | '8' | '9' | 'a' | 'b' | 'c' | 'd' | 'e' | 'f'; type Hex42 = `${HexDigit}${HexDigit}${Hex...

羅列はしたくないです(´・ω・｀)

まあそうですよね、そもそも不可能な気がする（TypeScript の型の再帰の制限に引っかかる）

changeset fix test and update snapshot fix: variables name test rename ...

Yutan0423 · 2023-09-27T11:00:50Z

１点、国際化対応どうしようかというのが気になりました！（遅くにすみません）
曜日をPropsで渡すしかなさそう...？

takurinton · 2023-09-27T11:02:28Z

@Yutan0423

確かに！！！
そうですねー、カレンダーと同じ対応になりそうかなと思ってます！

youchann · 2023-09-28T00:43:07Z

src/components/WeekTimeSelector/WeekTimeSelector.stories.tsx

+
+export const Example: StoryObj<WeekTimeSelectorProps> = {
+  args: {
+    weekTime: "ffffffffffffffffffffffffffffffffffffffffff",


あーこれね（Slackで議論していたのを見逃していた:pray:）

fluctのDBの構造に仕様が引っ張られすぎてる感がどうしても否めないな...:thinking:
プロダクトによってはhexを扱わないけど時間帯を指定したい時に、hexをどうにかせざるを得なくなる。

あきらめてhexを扱ってもらう

weektimeはstring[][]で扱う、hexへの変換ロジックは各プロダクトで書く

weektimeはstring[][]で扱う、hexへの変換ロジックはingred-uiに書いておく

3が妥協案で、2が自然だなという認識だけど、どう思う..?

それは結構思ってて、本当に hex 使わないといけないんだっけみたいなのは気になってました。（まさに Slack でも書いた通り、ヘルパーを含めるかどうか、hex を露出するかどうか）

3が妥協案で、2が自然だなという認識だけど、どう思う..?

ここに関しては同意です！
ただ、今回そのまま hex を使って実装したのは

現状このコンポーネントを使いたいプロダクトは同じ DB を参照しに行くプロダクト

hex への変換も同じロジックを使う

ユースケースが限定的

あたりを考えると一旦ここは hex を扱うのでもいいのかなと思います。拡張は後からいくらでもできると思うので。（特に3に倒すのはかなり容易なはず）

memo

hex については一旦このまま扱う形にする

現状のユースケースが hex を扱うものしかないので

今後、社内の別の場所で時間帯ターゲティングをする機会があり、なおかつ hex 以外の値を使用したいケースがあったときにその処理を切り出すことを検討する

takurinton · 2023-09-28T01:00:05Z

デザインの変更入った、仕様も固まってなかったし（hex 使うのか否かのあたり）ちょっとレビューリクエスト投げるには早かったかもしれない。ごめんなさい。(´・ω・｀)

takurinton · 2023-09-28T01:27:33Z

引数は hex に倒すことにした #1439 (comment)

takurinton · 2023-09-28T01:27:36Z

デザイン修正します
https://www.figma.com/file/fUeoFzCzL4rwRuW7CFW4m0/INGRED-UI?type=design&node-id=1671-1651&mode=design&t=8b3juMa8NxQ9O4Zh-0

youchann · 2023-09-28T01:30:29Z

src/components/WeekTimeSelector/WeekTimeSelector.tsx

+  errorText,
+  onChange,
+}) => {
+  const [weekTimeList, setWeekTimeList] = useState(getTargetSetting(weekTime));


weekTimepropsを変換したものがweekTimeListなので、状態として扱わないほうが実装として読みやすくなるかなーと思うのだけどどうだろう...??:thinking:

状態として扱わないほうが

変数、または useMemo とかを使うという意味ですか...??

なるほど？
weekTimeList は状態で扱うべきなのと、weekTime props を変換したものに関しては初期値なのでいいかなと思ってるのですがどうですかね？？？

weekTimeList は状態で扱うべきなの

そうではないなと思った理由が

weekTimepropsを変換したものがweekTimeListなので、状態として扱わないほうが実装として読みやすくなるかなーと思うのだけどどうだろう...??

なのだけど、状態で扱うべき理由はどこかな？:thinking:

説明がむずそうなら口頭でも

初期値は hex から weekTimeList を求める

更新は選択した index から weekTimeList を更新する

ので状態で表現するのが適切かなと思ってます。
setState してるのはここらへんで、これは getNewWeekTimeList を使って求めているので初期値とは別扱いかなと。　
https://github.com/voyagegroup/ingred-ui/pull/1439/files#diff-184eaa807c8a4513b51aa920881b647e71a985ee69e00c099ab96007b71dab88R43-R52

初期値は hex から weekTimeList を求める
更新は選択した index から weekTimeList を更新する

この辺は今の実装について教えてくれているけど、それが状態として扱うべき理由になるのがちょっとわからなかった...

古い引用だけど
https://ja.legacy.reactjs.org/docs/react-component.html

props を state にコピーしないでください。これはよくある間違いです。

この辺について言いたかった感じかな。

https://ja.legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-controlled-component
で、（Hexがややこしくしているけど）対応としては↑で良いんじゃないかな、という話。
Reactではよくある話だと思う。

別の手法も挙げてるけど、今回のケースだと1番目に挙げられた案で良いと思うぐらい、ユースケースはシンプルだと思う。

If you’re using derived state to memoize some computation based only on the current props, you don’t need derived state. See What about memoization? below.

なるほど、今回の例は「派生ステート」と呼ばれるのね。

ちょっと見てみます

難しかった、完全に理解した

youchann · 2023-09-28T01:43:42Z

src/components/WeekTimeSelector/WeekTimeSelector.tsx

+  const [hoverWeekTimeList, setHoverWeekTimeList] =
+    useState(defaultHoverWeekTime);
+
+  const [startIndex, setStartIndex] = useState<{
+    weekIndex: number;
+    timeIndex: number;
+  }>({ weekIndex: 0, timeIndex: 0 });
+  const [endIndex, setEndIndex] = useState<{
+    weekIndex: number;
+    timeIndex: number;
+  }>({ weekIndex: 0, timeIndex: 0 });


hoverWeekTimeListはstartIndexとendIndexから求めてる感じかな..?

実装ぱっと見hoverWeekTimeListはなくても実現できそうな雰囲気を感じてるのだけどどうだろう...?:thinking:

いや、hover は onMouseOver をしている button のインデックスをハンドラーで取得して管理しているので endIndex は見てないですね。

今の実装はそうだけど、hoverをしている（スタイルを変えたいセル）ってstartIndexとendIndex、要するに2つの座標情報さえあれば実現できそうな気がしたなという感じ。

だけど妙案出すには時間かかりそうだからこれでいこうか:pray:

結局それは hover の状態を計算しないといけないのでどっちにしても値は必要になる気がしました。

だけど妙案出すには時間かかりそうだからこれでいこうか🙏

🙇

@takurinton
AIとともに書いてみたけどどうだろうか、だいぶスッキリしたなーという印象がある。

import ErrorText from "../ErrorText"; import * as Styled from "./styled"; import React, { Fragment, useEffect, useMemo, useState } from "react"; import { timeList, weekList } from "./constants"; import { convertTargetSettingToHex, getNewWeekTimeList, getTargetSetting, } from "./utils"; export type WeekTimeSelectorProps = { weekTime: string; errorText?: string; onChange?: (weekTime: string) => void; }; const WeekTimeSelector: React.FC<WeekTimeSelectorProps> = ({ weekTime, errorText, onChange, }) => { const weekTimeList = useMemo(() => getTargetSetting(weekTime), [weekTime]); const [startIndex, setStartIndex] = useState<{ weekIndex: number; timeIndex: number; }>({ weekIndex: 0, timeIndex: 0 }); const [endIndex, setEndIndex] = useState<{ weekIndex: number; timeIndex: number; }>({ weekIndex: 0, timeIndex: 0 }); const [selectState, setSelectState] = useState<"none" | "choosing">("none"); const [toValue, setToValue] = useState<string>("1"); const handleChangeWeekTime = ( weekIndex: number, timeIndex: number, newValue: string, ) => { if (selectState === "choosing") { const newWeekTimeList = getNewWeekTimeList( startIndex.weekIndex, startIndex.timeIndex, weekIndex, timeIndex, newValue, [...weekTimeList], ); setSelectState("none"); onChange?.(convertTargetSettingToHex(newWeekTimeList)); } else if (selectState === "none") { setStartIndex({ weekIndex, timeIndex }); setEndIndex({ weekIndex, timeIndex }); setSelectState("choosing"); } }; // 上手く切り出してテスタブルにしたい const isWithinHoverRange = (weekIndex: number, timeIndex: number) => { if (selectState === "choosing") { const minWeekIndex = Math.min(startIndex.weekIndex, endIndex.weekIndex); const maxWeekIndex = Math.max(startIndex.weekIndex, endIndex.weekIndex); const minTimeIndex = Math.min(startIndex.timeIndex, endIndex.timeIndex); const maxTimeIndex = Math.max(startIndex.timeIndex, endIndex.timeIndex); return weekIndex >= minWeekIndex && weekIndex <= maxWeekIndex && timeIndex >= minTimeIndex && timeIndex <= maxTimeIndex; } return false; }; const handleMouseOver = (weekIndex: number, timeIndex: number) => () => { if (selectState === "none") { return; } setEndIndex({ weekIndex, timeIndex }); }; const handleMouseDown = (weekIndex: number, timeIndex: number, time: string) => () => { setStartIndex({ weekIndex, timeIndex }); setEndIndex({ weekIndex, timeIndex }); const newValue = time === "1" ? "0" : "1"; setToValue(newValue); handleChangeWeekTime(weekIndex, timeIndex, newValue); }; // MEMO: button 以外の場所で mouseup したときの挙動を制御する const onMouseUp = () => { // mousedown の位置が button のときにのみこの条件に到達する想定 if (selectState === "choosing") { handleChangeWeekTime(endIndex.weekIndex, endIndex.timeIndex, toValue); } }; useEffect(() => { addEventListener("mouseup", onMouseUp); return () => { removeEventListener("mouseup", onMouseUp); }; }); return ( <Styled.Container> <Styled.WeekTimeContainer> <Styled.EmptyContainer /> {timeList.map((time) => ( <Styled.TimeContainer key={time}>{time}</Styled.TimeContainer> ))} {weekTimeList.map((time, weekIndex) => ( // eslint-disable-next-line react/no-array-index-key <Fragment key={weekIndex}> <Styled.WeekContainer>{weekList[weekIndex]}</Styled.WeekContainer> {time.map((t, timeIndex) => ( <Styled.WeekTimeItem // eslint-disable-next-line react/no-array-index-key key={`${weekIndex}-${timeIndex}`} active={t === "1"} hover={isWithinHoverRange(weekIndex, timeIndex)} onMouseOver={handleMouseOver(weekIndex, timeIndex)} onMouseDown={handleMouseDown(weekIndex, timeIndex, t)} /> ))} </Fragment> ))} </Styled.WeekTimeContainer> {errorText && <ErrorText>{errorText}</ErrorText>} </Styled.Container> ); }; export default WeekTimeSelector;

すごい
hover の状態だけ少し修正したら使えそうですね

fm

この方向でやってみてもらえると:pray:

単一日を選択したいときに、onMouseDown で背景が水色になってしまうけど、まあそれは許容にしますか...。（許容というか、mousedown で日付を選択中（選択済みではなく）の場合は色が変わるという挙動を正しいものとする）

確認した、良さそう
https://cartaholdings.slack.com/archives/C02PN354W48/p1695881216676739?thread_ts=1695808495.681229&cid=C02PN354W48

hover の状態とはこれを指していたので、問題なさそう
#1439 (comment)

youchann · 2023-09-28T01:44:38Z

src/components/WeekTimeSelector/WeekTimeSelector.tsx

+  // 選択範囲内のセルをnewValueに更新する
+  const getNewWeekTimeList = (


（仕様としての）テストがあると読む人に優しいかなーと思った！

fix: style

test baseline update test

fix style

fix: state

This reverts commit 7bfc0bc.

takurinton force-pushed the feat-week-time-targeting branch from 372d6dc to e246e81 Compare September 27, 2023 09:12

takurinton marked this pull request as ready for review September 27, 2023 09:12

takurinton commented Sep 27, 2023

View reviewed changes

feat: <WeekTime /> component

daa9ccd

changeset fix test and update snapshot fix: variables name test rename ...

takurinton force-pushed the feat-week-time-targeting branch from c917a8a to daa9ccd Compare September 27, 2023 09:39

takurinton requested review from youchann and Yutan0423 September 27, 2023 09:42

youchann reviewed Sep 28, 2023

View reviewed changes

takurinton added 3 commits September 28, 2023 10:55

fix: weekList

63d2444

fix: style

getNewWeekTimeList move to utils.ts

c5fbf09

test baseline update test

fix style

841f313

fix style

takurinton force-pushed the feat-week-time-targeting branch from e2ed29b to 841f313 Compare September 28, 2023 04:43

takurinton added 2 commits September 28, 2023 13:51

update snapshot

abb1b1b

fix: state

fd0fcb5

fix: state

takurinton force-pushed the feat-week-time-targeting branch from 9c5d928 to fd0fcb5 Compare September 28, 2023 05:30

takurinton requested a review from youchann September 28, 2023 05:35

takurinton added 3 commits September 28, 2023 15:00

refactor

6d3c3cb

remove test

7bfc0bc

Revert "remove test"

7184d5b

This reverts commit 7bfc0bc.

takurinton requested review from youchann and removed request for youchann September 28, 2023 06:09

youchann approved these changes Sep 28, 2023

View reviewed changes

takurinton merged commit fc2d971 into master Sep 28, 2023
3 checks passed

takurinton deleted the feat-week-time-targeting branch September 28, 2023 06:15

FluctMember mentioned this pull request Sep 28, 2023

Version Packages #1440

Merged

		// 選択範囲内のセルをnewValueに更新する
		const getNewWeekTimeList = (

Feat week with time selector component #1439

Feat week with time selector component #1439

Conversation

takurinton commented Sep 27, 2023

changeset-bot bot commented Sep 27, 2023 • edited

🦋 Changeset detected

netlify bot commented Sep 27, 2023 • edited

✅ Deploy Preview for ingred-ui ready!

takurinton commented Sep 27, 2023 • edited

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

Yutan0423 commented Sep 27, 2023

takurinton commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takurinton commented Sep 28, 2023

takurinton commented Sep 28, 2023 • edited

takurinton commented Sep 28, 2023

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

youchann Sep 28, 2023 • edited

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

youchann Sep 28, 2023 • edited

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

takurinton Sep 28, 2023 • edited

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

changeset-bot bot commented Sep 27, 2023 •

edited

netlify bot commented Sep 27, 2023 •

edited

takurinton commented Sep 27, 2023 •

edited

takurinton commented Sep 28, 2023 •

edited

youchann Sep 28, 2023 •

edited

youchann Sep 28, 2023 •

edited

takurinton Sep 28, 2023 •

edited