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.

Sign up for GitHub

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

Jump to bottom

Feat <DualListBox /> component #1442

Merged
merged 29 commits into from
Oct 12, 2023
Merged

Feat <DualListBox /> component #1442

merged 29 commits into from
Oct 12, 2023

Conversation

takurinton
Copy link
Contributor

@takurinton takurinton commented Oct 2, 2023 •
edited

@changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2023 •
edited

🦋 Changeset detected

Latest commit: 66882d2

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

netlify bot commented Oct 2, 2023 •
edited

✅ Deploy Preview for ingred-ui ready!

Name Link
🔨 Latest commit 66882d2
🔍 Latest deploy log https://app.netlify.com/sites/ingred-ui/deploys/65274c4074839c0008c3ba81
😎 Deploy Preview https://deploy-preview-1442--ingred-ui.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.

@takurinton takurinton changed the title Feat <SelectableList /> component Feat <DualListBox /> component Oct 2, 2023
@takurinton
wip
60eb2d4 
fix handleAdd and handleRemove state

fix

fix directory

rename

checkbox

memo

fix
takurinton
takurinton commented Oct 4, 2023
View reviewed changes
src/components/SelectableList/DualListBox.tsx Outdated
items?: Item[];
};

export type UnselectedItem = Item & { selected?: boolean };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

selected は持たせたくないので剥がす方向で進めようかな

Copy link
Contributor Author

Choose a reason for hiding this comment

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

selected と unselected それぞれの list のみであれこれをしたい

Copy link
Contributor Author

Choose a reason for hiding this comment

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

対応した
e891459

@takurinton takurinton force-pushed the feat-selectable-list branch 2 times, most recently from 7f0401b to 589c87c Compare October 4, 2023 13:50
@takurinton
refactor and fix styles
d5aa835 
remove selected state

refactor

changeset

fix: style

upgrade snapshot

fix style

fix

fix: divider import

toggle

remove toggle checked and unchecked text

fix: style

rename
@takurinton takurinton marked this pull request as ready for review October 4, 2023 13:53
@takurinton takurinton force-pushed the feat-selectable-list branch 2 times, most recently from d38e97e to dd6f0ab Compare October 5, 2023 00:47
@takurinton
chore
cd047b9 
fix: style

fix type

forwardRef

rename

chore

test

export
takurinton
takurinton commented Oct 5, 2023
View reviewed changes
src/components/DualListBox/DualListBox.tsx Outdated
Comment on lines 8 to 42
export type ItemBase = {
id: string;
label: string;
};

export type ItemWithInverse = ItemBase & { isInverse: boolean } & {
items?: (ItemBase & { isInverse: boolean })[];
};
export type ItemWithoutInverse = ItemBase & { isInverse?: undefined } & {
items?: (ItemBase & { isInverse?: undefined })[];
};

export type Item = ItemWithInverse | ItemWithoutInverse;

/**
* @memo 内部で状態を保持するための型
*/
export type UnselectedItem = Item & { selected?: boolean };

type BaseProps = {
onAdd?: (id: string) => void;
onRemove?: (id: string) => void;
};

export type DualListBoxProps =
| (BaseProps & {
unselectedItems: ItemWithInverse[];
selectedItems: ItemWithInverse[];
onToggleInverse?: (id: string) => void;
})
| (BaseProps & {
unselectedItems: ItemWithoutInverse[];
selectedItems: ItemWithoutInverse[];
onToggleInverse?: undefined;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

これを担保するための型定義

  • onToggleInverse という prop が渡されたら Item の isInverse というプロパティは必須
  • onToggleInverse という prop が渡されなかったら Item の isInverse というプロパティは渡せない

@takurinton
Copy link
Contributor Author

デザインレビューはOK
https://cartaholdings.slack.com/archives/C02Q8F3FQBC/p1696478643633749?thread_ts=1696426873.677089&cid=C02Q8F3FQBC

youchann
youchann reviewed Oct 5, 2023
View reviewed changes
src/components/DualListBox/internal/UnselectedRenderer/UnselectedRenderer.tsx Outdated
<Accordion
style={{
borderTop: "none",
borderBottom: "none",
Copy link
Contributor

Choose a reason for hiding this comment

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

bottomの記述はなくてもよさそう？（オーバーライド元に記述がないような気がする

Copy link
Contributor Author

Choose a reason for hiding this comment

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

本当だ、直しますmm

src/components/DualListBox/internal/UnselectedRenderer/UnselectedRenderer.tsx Outdated
{item.items ? (
<Styled.AccordionWrapper key={item.id}>
<Accordion
style={{
Copy link
Contributor

Choose a reason for hiding this comment

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

styleだと「一体どこの...?」となるので、〇〇Styleみたいな命名の方が良いのかもね〜

DualListBox起因でオーバーライドするなら、DualListBox側でどうにかする方がよいな〜という気がしている。
Styled.UnselectedItemのborderを動的に変えるとか。
あるいはAccordionが存在する場合にはノンカテゴライズなitemを許容しないとか。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

あんまりいい方法が思いつかなかったのですが、こういうのどうですかね？

  • Accordion の border-top は Container で管理する
  • UnselectedRenderer（命名変えます）では styled.ts で Accordion コンポーネントを継承して border-top を打ち消す

57a9b00

こんな感じにしておくと、Accordion の挙動は保ったままスタイルの上書きができそう

Copy link
Contributor

Choose a reason for hiding this comment

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

それでもよさそう、どちらかというとStyled.UnselectedItem側が前のindexの要素をみてスタイルを変える方が良いかなーと思った。(例えば色を変えたりしていると「デザインシステムとしてどうなん？」となるが、今回はborderを消しているだけなので問題なさそう）

@youchann
Copy link
Contributor

youchann commented Oct 5, 2023

Accordion持ってくると角が潰れてるっぽい！

image

@takurinton takurinton requested review from youchann and removed request for youchann October 10, 2023 01:22
youchann
youchann reviewed Oct 10, 2023
View reviewed changes
src/components/DualListBox/DualListBox.tsx Outdated
Comment on lines 8 to 16
export type ItemBase = {
id: string;
label: string;
};

export type DualListBoxItemSelectedWithToggle = ItemBase & { checked: boolean };
export type DualListBoxItemSelectedWithoutToggle = ItemBase & {
checked?: undefined;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

selectedなitemたちってlabelないと困ることあるかな...?

type BaseProps = {
  onAdd?: (id: string) => void;
  onRemove?: (id: string) => void;
};

型的に更新時はidしかこないので、

  • DualListBoxItemSelectedWithToggle型を作るためにlabelを探す必要がある
  • 何かしらからmapして捜査する

手間があるから、labelない方が使う側としてはラクかなと思った！

Copy link
Contributor Author

Choose a reason for hiding this comment

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

label は表示用なので必要だと思うのですが、そういう意味ではなくてですか？
https://github.com/voyagegroup/ingred-ui/pull/1442/files#diff-af9a79e44ba16f954adf4cbd27213bbdba81410523de65fc30ff3ed96f55fbdfR32

Copy link
Contributor

@youchann youchann Oct 10, 2023 •
edited

Choose a reason for hiding this comment

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

ないほうが使う側として便利じゃないのかなーという話なので、そういう意味じゃないかも！

label自体は<DualListBox />で見つけ出せるよなーと思った！

#1442 (comment)

こちら側でよしなに管理できる内容のことをユーザー側で意識させたくないかなって

言葉を借りるとこんな感じかな。idさえあればlabelも引き当てられるしなぁ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

あー、選択候補から探し出せるからってことですかね？

Copy link
Contributor

@youchann youchann Oct 10, 2023 •
edited

Choose a reason for hiding this comment

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

そういうかんじ！

<DualListBox />内でやらない現状の実装だと、「選択候補から探し出す（labelを探す）」ことをユーザー側でやらなくちゃいけない気がする。

Copy link
Contributor

Choose a reason for hiding this comment

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

Toggleも考慮すると

export type selectedItems = {
  id: string;
  checked?: boolean;
};

こういう感じかな。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なんかその感じだと選択候補の方に checked は不要ということを想定しているんですかね

Copy link
Contributor Author

Choose a reason for hiding this comment

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

項目によってデフォルトで checked かどうかを選択候補リストから指定することを想定していましたが、それも違う形になりそう...??

Copy link
Contributor

@youchann youchann Oct 10, 2023 •
edited

Choose a reason for hiding this comment

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

なんかその感じだと選択候補の方に checked は不要ということを想定しているんですかね

たしかに。

#1442 (comment)
この辺の論点で「selectedItemsにlabelいらんやろ」という話をしようとしてただけだけど、風呂敷を広げるとそういう話にもなりそう。

項目によってデフォルトで checked かどうかを選択候補リストから指定することを想定していましたが、それも違う形になりそう...??

そうなりそう。
今の実装だとデフォルトの選択肢を定義する際にcandidateItemsとselectedItemsをよしなにしないといけないから、それはそれで使いづらいかもなと思った。

  • 候補リスト -> candidateItems
  • 選択情報 -> selectedItems

という棲み分けがシンプルで間違えにくいんじゃないかな...:thinking:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

今の実装だとデフォルトの選択肢を定義する際にcandidateItemsとselectedItemsをよしなにしないといけないから、それはそれで使いづらいかもなと思った。

まあ確かに、candidateItems と selectedItems の id と label は一致してる必要があるのでそれは使いづらいとは思いました。そこが仕組みで制御できるわけでもないし。

takurinton
takurinton commented Oct 11, 2023
View reviewed changes
src/components/DualListBox/DualListBox.tsx Outdated
@@ -7,7 +7,6 @@ import { useTheme } from "../../themes";

export type ItemBase = {
id: string;
label: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

label は持たないようにした

@takurinton
fix interface
2ba313b 
fix interface

refactor
@takurinton
Copy link
Contributor Author

takurinton commented Oct 11, 2023 •
edited

結局型としてはこんな感じ。

candidateItem

{
  id: string;
  label?: string;
  items?: 再帰[]
}

selectedItem

トグルなし

{
  id: string;
}

トグルあり

{
  id: string;
  checked?: boolean;
}

@takurinton
refactor
7800d3a 
...

fix

fix
youchann
youchann approved these changes Oct 12, 2023
View reviewed changes
src/components/DualListBox/index.ts Outdated Show resolved Hide resolved
takurinton and others added 3 commits October 12, 2023 10:12
@takurinton @youchann
Update src/components/DualListBox/index.ts
251df4c 
Co-authored-by: Yutaro Hayashi <yut.hayashi@cartahd.com>
@takurinton
format
a2f9931
@takurinton
fix: storybook
66882d2
@takurinton takurinton merged commit ce425fa into master Oct 12, 2023
3 checks passed
@takurinton takurinton deleted the feat-selectable-list branch October 12, 2023 01:34
@FluctMember FluctMember mentioned this pull request Oct 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youchann youchann youchann approved these changes

@deatiger deatiger Awaiting requested review from deatiger deatiger is a code owner

@Yutan0423 Yutan0423 Awaiting requested review from Yutan0423

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@takurinton @youchann