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.

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

dividerの色を変更&<Divider />を柔軟に #131

Merged
merged 6 commits into from
Jul 22, 2020
Merged

Conversation

youchann
Copy link
Contributor

@youchann youchann self-assigned this Jul 20, 2020
@youchann
Copy link
Contributor Author

youchann commented Jul 20, 2020

memo: 破壊的変更(<Divider />)

@youchann
Copy link
Contributor Author

youchann commented Jul 20, 2020

色の変更により変化のあるコンポーネント群

  • ButtonGroup
  • Checkbox
  • ContextMenu
  • DatePicker
  • DateRangePicker
  • Divider
  • DropdownButton
  • FixedPanel
  • Input
  • MenuList
  • Select
  • TextField
  • ToggleButton

※ snapshotが変化したコンポーネントをピックアップしています。

https://deploy-preview-131--ingred-ui.netlify.app/

@noronaoki
Copy link
Contributor

@yutaro1031

ContextMenu

これはそもそも影響受けてないかも?

DropDownButtonとMenuList

線がぶっとくなってるw
おそらくMenuList側がそうなってしまってるんだろう。
image

@youchann
Copy link
Contributor Author

@noronaoki

これはそもそも影響受けてないかも?

そうですね、dividerを用いていないので大丈夫でした!!

線がぶっとくなってるw

いい感じに戻しました!!w

@noronaoki
Copy link
Contributor

@yutaro1031
遅くにメンション飛ばしてすみませんmm
明日以降でいいですよ〜LGTM

</Typography>
<RowContainer>
<Divider variant="middle" />
<Divider m={3} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

margin加えたいときにSpacerで囲わないといけないの、微妙に不便なのでSpacerと同じpropsを入れられるようにしました。

Override Color
</Typography>
<RowContainer>
<Divider color="red" m={3} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dividerの色、利用する箇所によって変えたい時が多い雰囲気を感じたので、ここも自由に入れられるようにしています。


export const Divider = styled.hr<DividerProps>`
/* TODO: style.tsに書かれているものもutils/に移動せずに利用して良いのか相談 */
${spacer}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spacer()って、utils/とかに入れた方が良いですかねー??

Copy link
Contributor

Choose a reason for hiding this comment

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

そうね!他でも使うならSpacerUtils.tsとかに出すか!

Copy link
Contributor

@kinokoruumu kinokoruumu left a comment

Choose a reason for hiding this comment

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

LGTMです!

type Props = {
variant?: "fullWidth" | "middle";
type Props = SpacerProps & {
color?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

これはpaletteのkeyで縛ったりせずに何でも入れられていいってことかな?
例えば type="danger" | "primary" | ... とかにしてそれぞれのmainを利用するとかはいらない?

Copy link
Contributor Author

@youchann youchann Jul 21, 2020

Choose a reason for hiding this comment

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

@kinokoruumu

  1. <Divider />の色を変えたいがためにpalette.dividerを変更
  2. すると他のborderが薄くなり視認性が落ちた
  3. 薄くしたいのは、v2indexページと<MenuList /><hr />
  4. 場所によって色を変えたいニーズがあるのなら、<Divider />が自由に色定義できれば良いのでは??

みたいな経緯なんですよね。

type="danger" | "primary" | ... とかにしてそれぞれのmainを利用する

type IconColor = IconType | "active" | string;みたいな形式でも良いんですけど、それだったら初めからstringにしておいた方が開発側も利用者側もわかりやすい気がしているんですよねぇ...

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど!
たしかにそれだとstringの方がいいかも...

@@ -95,7 +95,7 @@ export const palette: Palette = {
active: colors.blue[100],
hint: colors.blue[50] as string, // TODO
},
divider: colors.basic[200],
divider: colors.basic[300],
Copy link
Contributor

Choose a reason for hiding this comment

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

お!ということは直打ちのやつほとんどなくなる???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに。。
borderに関してはなくせますね!!

boxShadowbackground-colorあたりにも使われているのですが、それらはまだ直打ちですね。。

Copy link
Contributor

Choose a reason for hiding this comment

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

このPRでも別PRでも大丈夫!

boxShadowやbackground-colorあたりにも使われているのですが、それらはまだ直打ちですね。。

これgray.mainを300にした方がいい気がしてきた
今検索した感じだと3箇所でしかgray.main使われてなくて、内2箇所はopacity変えてるから何とかなる気がする

Copy link
Contributor Author

Choose a reason for hiding this comment

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

borderの部分はpalette.dividerを使うようにしました!!

gray周りは頻繁に変えたりディスカッションしている気がするので、なんかもっと本質的な対応をガーデニングデーとかでやりたいよね。という話をnoroさんとしました。
#129 (comment)

@youchann youchann merged commit 1a94c4d into master Jul 22, 2020
@youchann youchann deleted the tweak-divider-color branch July 22, 2020 01:06
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.

None yet

3 participants