-
Notifications
You must be signed in to change notification settings - Fork 148
chore: チャートを色のみで表現しないようにした #5850
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.
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
base: master
Are you sure you want to change the base?
Conversation
commit: |
|
やること
|
Conflicts: packages/charts/package.json pnpm-lock.yaml
cd393d3 to
1546ed1
Compare
There was a problem hiding this comment.
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 adds pattern support for chart backgrounds using the @smarthr/patternomaly library to improve visual distinction between multiple datasets, particularly for accessibility. It also enhances legend styling to better represent line charts with custom line styles and point indicators.
- Integrated
@smarthr/patternomalyfor patterned backgrounds on bar charts - Customized legend rendering for line charts to show line styles with dash patterns
- Added various point styles and border dash patterns for line chart datasets
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added @smarthr/[email protected] dependency entries |
| packages/charts/package.json | Added @smarthr/patternomaly dependency |
| packages/charts/src/config/chartConfig.ts | Implemented pattern shapes, line styles, and enhanced legend/color configuration for different chart types |
| packages/charts/src/components/LineChart/LineChart.tsx | Updated getChartColors call to use new function signature with chart type parameter |
| packages/charts/src/components/BarChart/BarChart.tsx | Updated getChartColors call to use new function signature with chart type parameter |
| packages/charts/src/components/Chart/Chart.stories.tsx | Added story examples for line charts and multi-dataset scenarios |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const chartId = useId() | ||
| const chartRef = useRef<Chart<'bar'>>(null) | ||
| const chartColors = getChartColors<'bar'>(data.datasets.length) | ||
| const chartColors = getChartColors(data.datasets.length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(変更の内容と関係ないですが)ここも useMemo したほうが良い気がします。getChartColorsは毎回新しいオブジェクトを生成するので、後続するenhancedDataも呼び出しのたびに更新されてしまいます。
| const chartId = useId() | ||
| const chartRef = useRef<Chart<'line'>>(null) | ||
| const chartColors = getChartColors<'line'>(data.datasets.length) | ||
| const chartColors = getLineChartColors(data.datasets.length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここも同様です
| /** Lineチャートのレジェンドはポインターではなく線にしたいが、lineDash を簡単に指定できないため generateLabels を使っている */ | ||
| /** 折れ線グラフはレジェンドを線+ポイントにしたい */ | ||
| const generateLegendOptions = <TType extends ChartType>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この2つめのコメントってTODOっていう理解であっていますか?本当はポイントと線を組み合わせて5×5=25通りまで表現できるんだけど、レジェンドでどちらかしか表示できないから5通りでダブりが出てしまうという感じですかね?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODOです! prefix つけておきます!
ダブりが出てしまうのと、わかりやすさのためにポイントも変えているのにレジェンドには表示できないのは意味が半減してしまうのでつけられるようにしたいです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
どっちかしか出せないならポイントのほうが判別しやすいかな〜〜と思いました、好みの問題だとおもいますが!
| chart.data.datasets.map((dataset, index) => ({ | ||
| text: dataset.label, | ||
| strokeStyle: CHART_COLORS[index % CHART_COLORS.length], | ||
| lineDash: BORDER_DASHES[index % BORDER_DASHES.length], | ||
| lineWidth: 4, | ||
| pointStyle: 'line', | ||
| })), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(感想)ここ理解するのがむずかったです。ボーダーのスタイルはChartDataで指定するのに、レジェンドのスタイルはChartOptionsの側で指定することになっているから、ボーダーとレジェンドで同じ感じのスタイルにしようとしたらチャートのインスタンスを持ってこなきゃいけない……みたいなことだと理解しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:wakaru: ちょっとどうにかできないか考えます
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感想なのでこのままでも大丈夫です!
| @@ -0,0 +1,68 @@ | |||
| import { draw } from '@smarthr/patternomaly' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(モジュール名について)config/colors.tsというのは混乱のもとになりそうです。config/chartConfig.tsの中でもレジェンドやツールチップなどの色の話をしているからです。
helpers/options.ts と helpers/data.tsのようにすれば型名ともマッチしていて、どこに何が置いてあるのかわかりやすそうだと思いました
関連URL
なし
概要
チャートの色やポイントの形を、色のみで判断しなくても良いようにいろいろと変更を加えました。
パターンをつけるには patternomaly というライブラリをフォークしパッケージをアップデートしたものを使用しています。
https://github.com/kufu/patternomaly
変更内容
棒グラフ系の背景色で語るグラフに、パターンがつくようになりました
パターンが入ることで、グラフが色のみで情報を提示しない様になりました。
似たパターンが隣り合わないように順番を調節しています。
グラフのデータ量が多くなるとそれでも見えにくくなりますが、ルールによってデータ量に関するものも入れる予定です。
線グラフの線の種類とポイントが変わるようにしました。
これも色のみで情報を提示しないように変更しました。
現状5個以上のデータを追加すると同じパターンがでてしまいます。
確認方法
まだプレビュー環境を用意していなくて申し訳ないです!