Skip to content

Sliderの修正 - #137

Merged
touyou merged 16 commits into
mainfrom
fix/slider
Sep 19, 2025
Merged

Sliderの修正#137
touyou merged 16 commits into
mainfrom
fix/slider

Conversation

@touyou

@touyou touyou commented Aug 26, 2025

Copy link
Copy Markdown
Member

概要

指摘事項と調整です

https://www.notion.so/goodpatch-design-div/Slider-4e3de10abcde47d5977ba734eeac4335?source=copy_link

変更内容

  • Disabledの修正
  • 入力内容の値を表示
  • テストの追加
  • Storybookの調整
  • registryの調整
  • Figma Connectの設定

動作確認

  • pnpm lint を実行
  • pnpm build:package を実行してパッケージが正常にビルドされることを確認
  • pnpm test を実行してテストがすべて成功することを確認

備考

  • Figmaのプロパティとの対応をちゃんと取るのが難しかったのでFigma Connectは最低限です
  • そしてFigma側の入力内容表示もしや間違ってます...?間違ってそうならvalueを数字にしてもらってunitを実装同様にするみたいな対応をやってもらってもいいかもですね
image

- スライダーの初期値、最大値、増減単位、無効化、向き、スタイルに関する説明を削除
- スライダーの範囲の背景色を変更
- スライダーのトラックスタイルを更新
@touyou touyou self-assigned this Aug 26, 2025
@vercel

vercel Bot commented Aug 26, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
sparkle-design Ready Ready Preview Comment Aug 29, 2025 4:59am

- スライダーの制御機能を追加し、状態管理を実装
- スライダーに単位表示を追加
- 不要なプロパティ「orientation」を削除
- スライダーのスタイルを改善
- スライダーコンポーネントの制御状態を管理するロジックを改善
- 非制御コンポーネントの内部状態管理を追加
- スライダーコンポーネントのテストケースを新規作成
- ユーザーインタラクションに関するテストを強化
- スライダーコンポーネントの説明をより具体的に修正
- Figmaとの接続設定を新規追加
@touyou
touyou requested a review from ore0 August 27, 2025 02:44
@touyou
touyou marked this pull request as ready for review August 27, 2025 02:44
Copilot AI review requested due to automatic review settings August 27, 2025 02:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements improvements to the Slider component based on feedback, including disabled state fixes, value display, comprehensive test coverage, and Storybook adjustments.

  • Added value indicator display with optional unit support
  • Fixed disabled state styling for track, range, and thumb elements
  • Added comprehensive test coverage with 503 lines of tests
  • Enhanced Storybook with controlled mode and unit examples

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/test/setup.ts Added ResizeObserver and MatchMedia polyfills for jsdom testing environment
src/components/ui/slider/item.json Updated component description and added sparkle-font dependency
src/components/ui/slider/index.tsx Major refactor with value display, improved disabled styling, and controlled/uncontrolled mode support
src/components/ui/slider/index.test.tsx Added comprehensive test suite covering all component functionality
src/components/ui/slider/index.stories.tsx Updated Storybook with new controlled and unit examples
src/components/ui/slider/index.figma.tsx Added basic Figma Connect configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/components/ui/slider/index.tsx Outdated
Comment on lines +144 to +145
const [internalValue, setInternalValue] = React.useState<number[]>(
defaultValue || [0]

Copilot AI Aug 27, 2025

Copy link

Choose a reason for hiding this comment

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

The fallback value [0] may not respect the component's min prop. If min is greater than 0, the initial state will be invalid. Consider using [min || 0] instead.

Suggested change
const [internalValue, setInternalValue] = React.useState<number[]>(
defaultValue || [0]
defaultValue || [props.min ?? 0]

Copilot uses AI. Check for mistakes.
Comment thread src/components/ui/slider/index.tsx Outdated
- スライダーのデフォルト値を `defaultValue` または `props.min` に変更

@ore0 ore0 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

unit: "%",
},
render: ({ className, ...props }) => (
<div className="flex flex-col items-center justify-center w-[240px]">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

240pxが指定されている影響でunitを指定すると単位が落ちちゃっていました
image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ore0 shrink-0が必要でした🙏
修正しました!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

対応ありがとうございます!
桁が変わるタイミングでslider本体が縮んでしまっていました😭
https://github.com/user-attachments/assets/cb9d8988-a035-42e0-b0f2-8625b6dec3d6

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

あーなるほど...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ore0 今pushした状態で一応マルチバイト一文字+数字3文字ならスライダーの伸び縮みなくできそうな感じになりました!
これ思ったんですけどガタガタするので数字をmonospaceにできたらいいかなぁと思い始めたんですがどうでしょう?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ore0 🎉
モノスペース右揃えでならうまくいきました

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

やっぱり桁が変わるタイミングで絶妙にタブがズレちゃって惜しいんですよね...
Figmaでも表現できないポイントなので、方針自体をすり合わせた方が良さそうです。
別の方法で値を表示することも検討した方がいいかもしれないです。
https://github.com/user-attachments/assets/a807cc81-5261-489c-8a50-7a14ba5adfab

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ore0 これは自分が気づかなかった箇所なので最小幅を調整すれば防げるかなと思うのですがどうでしょう?
計算自体はvibe codingしたんでchなんてあるんだなーという気持ちで使ってたんですが以下見る感じ親要素の文字幅でやってるみたいなのでちょっとロジック調整するはやっても良さそうです

https://web.havincoffee.com/css/value/length/ch.html

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

あとそれでいうと挙動的にthumbの幅を想定できていないかも?です

@touyou touyou Aug 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ore0 今pushしたバージョンであれば自分の目視したところ解消できていると思うのですがいかがでしょう?
Thumbの設定値を元に調整しているので計算としても意味のあるものになっているかと思います(あと--spacingを使うことで設定が変わったシステムでも動きます)

touyou added 2 commits August 27, 2025 14:03
- スライダーコンポーネントの表示幅を240pxから300pxに変更
- スライダーコンポーネントの幅を300pxから240pxに変更
- スライダーのレンダリングに関連するスタイルを調整
- スライダーコンポーネントの状態を更新し、テスト結果を反映しました。
- スライダーコンポーネントの関数形式を変更
- 不要なコードを削除し、可読性を向上
- スライダーの内部状態管理を整理
- JSX構造を簡素化し、データ属性を追加
- スライダーのルートスタイルを変更し、最小幅を設定
- スライダーのスパン要素の最小幅を変更し、フレックスシンクを適用
- 値インディケーターのクラス名を "min-w-10" から "min-w-14" に変更
touyou added 2 commits August 29, 2025 10:15
- スライダーコンポーネントのテキストアラインメントを左揃えから右揃えに変更
- 値インディケーターの最小幅を桁数と単位に基づいて計算する機能を追加
- スライダーコンポーネントのスタイルを改善
- ヘルパー関数を追加して、最小幅の計算を行う
- 値インディケーターの最小幅を計算式を用いて設定
- スライダーのユニットに応じた最小幅の調整を実施

@ore0 ore0 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTMです!
調整ありがとうございます🙇‍♂️🙇‍♂️🙇‍♂️

@touyou
touyou merged commit 927239f into main Sep 19, 2025
3 checks passed
@touyou
touyou deleted the fix/slider branch September 19, 2025 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants