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

fix(MultiComboBox): item.labelがReactNodeだった場合のitemの比較を内部の文字列によって行うように #5191

Merged
merged 5 commits into from
Feb 5, 2025
Merged

fix(MultiComboBox): item.labelがReactNodeだった場合のitemの比較を内部の文字列によって行うように #5191

merged 5 commits into from
Feb 5, 2025

Conversation

eatski
Copy link
Contributor

@eatski eatski commented Dec 13, 2024 •
edited
Loading

関連URL

https://kufuinc.slack.com/archives/C06KVP5PL23/p1734060131150619

概要

close #5190

MultiComboBoxのitem同士が同一かどうかの判定でlabelも参照されているが、labelに設定できるJSX.Elementは等価であっても等値ではない場合がありうるので、label同士の === による判定が意図通りになるかはアプリケーションの実装による。
この判定が意図通りにならない場合、同じアイテムを複数回選択できてしまう・一度選択したアイテムを選択解除できないなどの問題が発生しそう

変更内容

valueがitems内で一意（valueが同じでlabelが違うitemは無い）という前提を置き、MultiComboBox内でitem同士が同一かどうかの判定を行っていた部分でlabelも参照するのをやめました。

item.labelがReactNodeだった場合のitemの比較を内部の文字列によって行うようにしました。

確認方法

@eatski eatski requested a review from a team as a code owner December 13, 2024 07:12
@eatski eatski requested review from moshisora and uknmr and removed request for a team December 13, 2024 07:12
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Dec 13, 2024 •
edited
Loading

Open in Stackblitz

npm i https://pkg.pr.new/kufu/smarthr-ui@5191

commit: 878d6f5

eatski
eatski commented Dec 13, 2024
View reviewed changes
packages/smarthr-ui/src/components/ComboBox/MultiComboBox.tsx Outdated
@@ -468,7 +462,7 @@ const ActualMultiComboBox = <T,>(
className={selectedListStyle}
>
{selectedItems.map((selectedItem, i) => (
<li key={`${selectedItem.label}-${selectedItem.value}`}>
<li key={selectedItem.value}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

valueがitems内で一意（valueが同じでlabelが違うitemは無い）という前提を置いたので、keyの設定にlabelは必要ないはず
また、JSX.Elementが渡されると [Object Object] が展開されてしまうので修正しました。

uknmr
uknmr approved these changes Jan 7, 2025
View reviewed changes
Copy link
Collaborator

@uknmr uknmr left a comment

Choose a reason for hiding this comment

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

LGTM! ありがとうございます!!

oti
oti requested changes Jan 7, 2025
View reviewed changes
packages/smarthr-ui/src/components/ComboBox/MultiComboBox.tsx Outdated
const matchedSelectedItem = selectedItems.find(
(item) => item.label === selected.label && item.value === selected.value,
)
const matchedSelectedItem = selectedItems.find((item) => item.value === selected.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

ラベルがReactNodeの場合（ダミー……テキスト） を選択しても、ドロップダウン側で選択済みスタイルが当たってないようでした。
どう直せばいいかまで検証できていないのですが、いったんコメントしておきます 🙇

スクリーンショット 2025-01-07 17 54 03

Copy link
Contributor Author

@eatski eatski Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@oti
確かにそうですね汗

こちら修正前も発生はしていたのでデグレというわけではないのですが一緒に直してしまいました〜
ただ、この修正はSingleComboBoxにも波及してます（おそらく同様のバグが修正されているはず）ので軽く動作確認はしておきましたがそちらも問題なさそうでした。

Copy link
Contributor

Choose a reason for hiding this comment

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

選択済みスタイルについての修正はこちらも確認できました！ありがとうございます！　

eatski
eatski commented Jan 7, 2025
View reviewed changes
Copy link
Contributor Author

@eatski eatski left a comment

Choose a reason for hiding this comment

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

いただいた指摘について対応いたしました！

packages/smarthr-ui/src/components/ComboBox/MultiComboBox.tsx Outdated
const matchedSelectedItem = selectedItems.find(
(item) => item.label === selected.label && item.value === selected.value,
)
const matchedSelectedItem = selectedItems.find((item) => item.value === selected.value)
Copy link
Contributor Author

@eatski eatski Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@oti
確かにそうですね汗

こちら修正前も発生はしていたのでデグレというわけではないのですが一緒に直してしまいました〜
ただ、この修正はSingleComboBoxにも波及してます（おそらく同様のバグが修正されているはず）ので軽く動作確認はしておきましたがそちらも問題なさそうでした。

@eatski eatski force-pushed the fix/combobox-item-compare-logic branch from 9e72b87 to e678fb1 Compare January 7, 2025 09:37
@eatski
Copy link
Contributor Author

eatski commented Jan 7, 2025

テスト落ちてますが、テストの意図するところが不明なので確認中です

@eatski
Copy link
Contributor Author

eatski commented Jan 7, 2025

failしているテストケースについて、blameしてみたところこのPRが見つかりました。
どうやらバグを再現するためのテストケースであり正常な動作を担保するためのものではなさそうなので、修正に伴いテストの内容も変更いたしました。

#4346

@AtsushiM
Copy link
Member

AtsushiM commented Jan 7, 2025 •
edited
Loading

valueがitems内で一意（valueが同じでlabelが違うitemは無い）という前提

これは大体そうだろうなぁと思いつつ、たまにあるし落とすべきかどうかちょっと迷いますね...
smarthr-uiはreact-innertextを導入しているので対象からテキスト引っこ抜いて比較するという手もありますが、やりすぎっすかね？
https://www.npmjs.com/package/react-innertext

俺個人の意見としては

  • valueが同じ、labelが別、という場合はほぼないだろうけどないわけではない
  • 上記状況に陥った場合、開発者が問題に気づきにくそう

辺りで可能なら堅い判定したほうがいいんじゃないかな〜派です

@uknmr uknmr self-requested a review January 8, 2025 04:49
@uknmr
Copy link
Collaborator

uknmr commented Jan 8, 2025

対象からテキスト引っこ抜いて比較

良さそう!

@eatski
Copy link
Contributor Author

eatski commented Jan 10, 2025

@AtsushiM

なるほど、アリだと思います〜
そのように対応します！

@eatski eatski force-pushed the fix/combobox-item-compare-logic branch from c96c364 to 01b0af2 Compare January 10, 2025 12:41
@eatski eatski force-pushed the fix/combobox-item-compare-logic branch from a0d1da6 to d59de7c Compare January 20, 2025 10:01
@eatski eatski force-pushed the fix/combobox-item-compare-logic branch from d59de7c to 878d6f5 Compare January 20, 2025 10:01
@eatski eatski changed the title fix(MultiComboBox): itemの比較にitem.labelを参照していたのをやめる fix(MultiComboBox): item.labelがReactNodeだった場合のitemの比較を内部の文字列によって行うように Jan 20, 2025
@eatski
Copy link
Contributor Author

eatski commented Jan 20, 2025

@AtsushiM
こちら修正しました〜

@oti oti self-requested a review January 30, 2025 07:05
oti
oti approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@oti oti left a comment

Choose a reason for hiding this comment

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

LGTM!

@eatski eatski merged commit 547d2e5 into master Feb 5, 2025
15 checks passed
@eatski eatski deleted the fix/combobox-item-compare-logic branch February 5, 2025 04:49
@Qs-F Qs-F mentioned this pull request Feb 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oti oti oti approved these changes

@moshisora moshisora Awaiting requested review from moshisora moshisora is a code owner automatically assigned from kufu/group-dev-smarthr-ui-reviewers

@uknmr uknmr Awaiting requested review from uknmr

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

Successfully merging this pull request may close these issues.
4 participants
@eatski @AtsushiM @uknmr @oti