Notification Badgeの追加 #31

youchann · 2020-05-19T09:55:45Z

No description provided.

youchann · 2020-05-19T12:12:39Z

src/components/NotificationBadge/NotificationBadge.tsx

+export type Props = {
+  content?: number | string;
+  position?: Styled.BadgeProps["position"];
+  size?: Styled.BadgeProps["size"];
+  max?: number;
+};


依存の方向を１つにしたいので、styled.tsに型を置いている。。

型はNotificationBadge.tsxに置きたいな
styled側で三項演算子でstyle変えてるけど、判定後の値を渡すのではダメなんだろうか？

参考

ingred-ui/src/components/Button/Button.tsx

Line 123 in 1633295

fontWeight={color === "cancel" ? "normal" : "bold"}

youchann · 2020-05-19T12:19:01Z

src/components/NotificationBadge/styled.ts

+  background-color: ${({ theme }) => theme.palette.danger.main};
+  font-size: 0.75rem;
+  font-weight: bold;
+  z-index: 1;


<Container />でz-index指定していないので、全体に影響することはない。
Material-UIのBadgeで指定されていたから追加したけど、いらないような...:thinking:

不要なら消そう？

youchann · 2020-05-19T12:20:49Z

src/components/NotificationBadge/styled.ts

+  border-radius: 10rem;
+  color: ${({ theme }) => theme.palette.text.white};
+  background-color: ${({ theme }) => theme.palette.danger.main};
+  font-size: 0.75rem;


今回はMaterial-UIにならってrem指定なのですが、相対値指定でのデメリットってありますかね...?

ingred-ui上でremにしないといけない理由があるのならそれで大丈夫だよ
inngred-uiではhtmlのfont-sizeに干渉しないので意図したsizeになるかは気になる

htmlのfont-sizeに干渉しないので意図したsizeになるかは気になる

確かにそうなので、pxで固定します！！

size変えたいニーズが出てきた時点で、修正を加える予定。

youchann · 2020-05-19T12:27:01Z

src/components/NotificationBadge/styled.ts

+    variant === "normal" ? "20px" : dotSizeMapping[size]};
+  min-width: ${({ variant, size }) =>
+    variant === "normal" ? "20px" : dotSizeMapping[size]};
+  padding: 0 ${({ variant }) => (variant === "normal" ? "6px" : 0)} 1px;


末尾の1pxは指摘があって当てたのですが↓
https://cartaholdings.slack.com/archives/CKYAWNNLX/p1589886510429700?thread_ts=1589882789.427700&cid=CKYAWNNLX

なぜ1pxずれるのかがわからない....

line-height: 1にしたら直ったりしないかな？

直らなかったです...

他の要素に影響はなさそうなので、このままでも良いかなという判断。

kinokoruumu · 2020-05-20T06:24:08Z

バッジの中心が指定ポジションに来るっていうので合ってるのかな？
バッジの端が指定ポジションに来る方がいい気がした

youchann · 2020-05-20T07:09:58Z

@kinokoruumu

※以降もコメは返していきますが、急ぎではないので返信は明日でも大丈夫です!!一気に評価会資料書いちゃってください!!!

端を軸にすると、ある程度大きい要素にBadgeつけた時に違和感がある気がしています...

Material-UIも同じ
長い文字列を入れるケースは少なそう

なので、現行のままでいきたいです!!!

kinokoruumu · 2020-05-20T07:39:32Z

@kinokoruumu

※以降もコメは返していきますが、急ぎではないので返信は明日でも大丈夫です!!一気に評価会資料書いちゃってください!!!

ありがとう！！！:sob::sob::sob:
資料落ち着いたら見ます！！

youchann · 2020-05-22T00:36:19Z

@kinokoruumu

Drawerをingred-uiに乗っける時のココ↓に使えそうなので、お手隙でみていただけると:bow:

kinokoruumu · 2020-05-22T05:20:25Z

sizeがdotの時しか反映されないのが気になった
そもそもdotのサイズって可変にしたんだっけ？

kinokoruumu · 2020-05-22T05:24:02Z

src/components/NotificationBadge/styled.ts

+  vertical-align: middle;
+`;
+
+export type BadgeProps = {


exportしてるけど使ってなさそう？

使わなくなって、対応漏れですね...

youchann · 2020-05-22T06:24:34Z

sizeがdotの時しか反映されないのが気になった
そもそもdotのサイズって可変にしたんだっけ？

XDに3サイズ書かれてあったので、可変だという判断でした。
自分的にはあまり気にならないのですが、不変にした方がいいですかね....??

youchann

ほぼMaterial-uiになりました。

youchann · 2020-05-22T08:12:37Z

src/components/NotificationBadge/styled.ts

+  line-height: 1;
+  padding: 0 ${({ variant }) => (variant === "normal" ? "6px" : 0)};


line-height当てたんですけど、変わってますかね....?

僕見た感じ、変わってなくて...

youchann · 2020-05-22T08:13:06Z

src/components/NotificationBadge/styled.ts

  border-radius: 10rem;
  color: ${({ theme }) => theme.palette.text.white};
  background-color: ${({ theme }) => theme.palette.danger.main};
  font-size: 10.5px;
  font-weight: bold;
+  transition: transform 0.3s;


badgeが消える時にアニメーションがつくようになりました

youchann · 2020-05-22T08:14:40Z

@kinokoruumu お手隙でご確認ください:bow:

youchann · 2020-05-26T09:33:50Z

@kinokoruumu
すいません、line-heightが反映されているかアンケート取ろうという話でしたが、ブランチ環境だと自分のPCでもしっかり反映されていました。

よって、現行のままで問題ないと思います:bow:

kinokoruumu · 2020-05-26T09:41:21Z

src/components/NotificationBadge/NotificationBadge.tsx

+    | {
+        variant: "normal";
+        badgeContent: number;
+        dotSize?: never;


なんでneverにしてるんだろう？

あ、この辺variantをoptionalにしない代わりに型を動的に変えるようにしてみました。

上のコードだと「variantがnormalの時にはdotSizeを定義できない」ようにしたいのでnever型が適切だと考えたのですが、他に良い型あったりしますかね...??

OmitかExcludeで実現できないだろうか？

youchann · 2020-05-27T07:10:04Z

export type Props =
  | ({
      variant: "dot";
    } & Omit<BaseProps, "BadgeContent" | "max" | "showZero">)
  | ({
      variant: "normal";
      badgeContent: number;
    } & Omit<BaseProps, "dotSize">)
  | ({
      variant: "normal";
      badgeContent: string;
    } & Omit<BaseProps, "max" | "dotSize" | "showZero">);

これだとmax,dotSize,showZeroがpropsの引数に入れられないと怒られたので、Excludeを使ってみる。(Excludeは結局never型を当てるらしいけども...

youchann · 2020-05-27T07:17:30Z

variantによって他のキーを使う/使わないを決めたいので、Excludeは今回の目的にそぐわないかもしれない。

youchann · 2020-05-27T07:26:40Z

Omit -> 期待通りの動作にならない
Exclude -> 今回の利用法にはそぐわない

ということで元に戻しました。

kinokoruumu

neverはちょっと自分でも調べてみる！
このPRとしてはLGTMです！

youchann added the enhancement New feature or request label May 19, 2020

youchann self-assigned this May 19, 2020

youchann commented May 19, 2020

View reviewed changes

youchann requested a review from kinokoruumu May 20, 2020 00:43

youchann added 5 commits May 22, 2020 09:38

Notification Badgeの追加

4c24e9e

tweak

56bbfa6

tweak

9056331

tweak

5135f65

tweak

f91bf09

youchann force-pushed the add-notification-badge branch from 0257e1f to f91bf09 Compare May 22, 2020 00:39

tweak

6e0da47

kinokoruumu reviewed May 22, 2020

View reviewed changes

tweak

7124cd6

youchann requested a review from kinokoruumu May 22, 2020 06:25

youchann added 2 commits May 22, 2020 17:06

プロパティを色々変えた

eb68a73

tweak

67e6e70

youchann commented May 22, 2020

View reviewed changes

youchann added 2 commits May 24, 2020 12:20

型を強くしてみた

d1b565e

test修正

e4e2d38

kinokoruumu reviewed May 26, 2020

View reviewed changes

props周りを元に戻す

c182aef

youchann requested a review from kinokoruumu May 27, 2020 07:27

kinokoruumu approved these changes May 27, 2020

View reviewed changes

youchann added 2 commits May 27, 2020 17:14

Merge branch 'master' into add-notification-badge

978b04f

knobs入れ忘れ

0015a68

youchann merged commit 21ca6e3 into master May 27, 2020

youchann deleted the add-notification-badge branch May 27, 2020 08:54

youchann mentioned this pull request May 27, 2020

@storybook/addon-knobs の設定漏れを修正 #44

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification Badgeの追加 #31

Notification Badgeの追加 #31

youchann commented May 19, 2020

youchann May 19, 2020

kinokoruumu May 20, 2020

youchann May 19, 2020

kinokoruumu May 20, 2020

youchann May 19, 2020

kinokoruumu May 20, 2020

youchann May 20, 2020

youchann May 19, 2020

kinokoruumu May 22, 2020

youchann May 22, 2020

kinokoruumu commented May 20, 2020

youchann commented May 20, 2020

kinokoruumu commented May 20, 2020 •

edited

youchann commented May 22, 2020

kinokoruumu commented May 22, 2020

kinokoruumu May 22, 2020

youchann May 22, 2020

youchann commented May 22, 2020

youchann left a comment

youchann May 22, 2020 •

edited

youchann May 22, 2020

youchann commented May 22, 2020

youchann commented May 26, 2020

kinokoruumu May 26, 2020

youchann May 26, 2020

kinokoruumu May 27, 2020

youchann commented May 27, 2020

youchann commented May 27, 2020

youchann commented May 27, 2020

kinokoruumu left a comment

		line-height: 1;
		padding: 0 ${({ variant }) => (variant === "normal" ? "6px" : 0)};

Notification Badgeの追加 #31

Notification Badgeの追加 #31

Conversation

youchann commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinokoruumu commented May 20, 2020

youchann commented May 20, 2020

kinokoruumu commented May 20, 2020 • edited

youchann commented May 22, 2020

kinokoruumu commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youchann commented May 22, 2020

youchann left a comment

Choose a reason for hiding this comment

youchann May 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youchann commented May 22, 2020

youchann commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youchann commented May 27, 2020

youchann commented May 27, 2020

youchann commented May 27, 2020

kinokoruumu left a comment

Choose a reason for hiding this comment

kinokoruumu commented May 20, 2020 •

edited

youchann May 22, 2020 •

edited