-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feat/update toast #482
Feat/update toast #482
Conversation
reg-suit detected visual differences. Check this report, and review them. 🔴🔴🔴🔴🔴 What do the circles mean?The number of circles represent the number of changed images.🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items How can I change the check status?If reviewers approve this PR, the reg context status will be green automatically. |
Visit the preview URL for this PR (updated for commit d1dd07a): https://ameba-spindle--pr482-feat-update-toast-jyt8819o.web.app (expires Sat, 17 Sep 2022 03:25:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
b341f15
to
7a24bef
Compare
Toastのbundlesizeが1.1kbを超えてしまった、、 |
7a24bef
to
2e06cf0
Compare
@@ -1,5 +1,31 @@ | |||
:root { | |||
--Toast-z-index: 1; | |||
|
|||
/* Information */ | |||
--Toast-IconButton--information-color: var( |
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.
Q: スタイルをカスタマイズできる要望は結構ありそうですかな。Toastはvariantである程度スタイル分かれてるのでそれでよかったりしないか確認したいかもです!
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.
カスタマイズできなくて良さそうでした:pray:
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.
修正しました!
757ae1e
"path": "./dist/Toast/*.mjs", | ||
"maxSize": "1.5 kB" |
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.
Toastのmjsだけbundlesize増やしました
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.
くぅ、惜しいw
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.
947b71f
ああ、、コンフリクトの解決おかしくなっていたので修正しました:bow:
@@ -1,6 +1,7 @@ | |||
import React, { useCallback, useEffect, useRef, useState } from 'react'; | |||
import { CheckCircleFill, CrossBold, Information } from '../Icon'; | |||
import { IconButton } from '../IconButton'; | |||
import '../IconButton/IconButton.css'; |
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.
Dialogと同様にIconButton.cssはToast内で読み込むようにしました
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.
fa3995f
cssで読み込むようにしました
hasError?: boolean; | ||
}; | ||
} & ( | ||
| { variant: Variant['information']; hasInfoIcon: boolean } |
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.
(他の箇所での実例がまだなさそう?だけど、) 表示、非表示系のbooleanはhide*
, show* or display*
にしようかと思っているのですがどでしょう。あと以下の前提だと、表示がデフォルトでもよさそうですが、その辺仕様確認したいです〜。
variant="information"の時は非表示できます
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.
あとも一点、「informationのときにだけアイコン消せる」仕様の重要度や意図確認しておきたいです!重要度が高ければこのままコードでも担保するでよしで、あくまでガイドレベルであればvariant関わらず利用時に指定するでもいいかもしれないです。
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.
f8dc16d
修正しました!
variant={hasError ? 'danger' : 'outlined'} | ||
onClick={() => setMessage('Toast Content')} | ||
variant={variant === "error" ? 'danger' : 'outlined'} | ||
onClick={() => setMessage(_message || 'Toast Content')} |
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.
今更で恐縮なのですが、Storyのテキストをより実態に合わせた日本語にしていこうプロジェクト #162 がありまして、程よいタイミングで変えていきたいデス!Figmaの例に合わせても良い気がしてます。
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.
8aa0ceb
修正しました!
ref={transitionElementRef} | ||
> | ||
<div | ||
className={`${BLOCK_NAME}--content ${errorContentClassName}`} | ||
className={`${BLOCK_NAME}-content ${BLOCK_NAME}-content--${variant}`} | ||
tabIndex={0} |
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.
Q: ここをフォーカスする機能はなくしてもいいかなっていう話があった気がしますが、今回含めず別途にしますかな
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.
この状態で出すのも微妙な気がするのでこのPRで修正しちゃいます:pray:
#482 (comment)
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.
5bc833c
focus外しました!
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.
💬
17169ce
to
27cd5bd
Compare
position=bottomが動いてなかったので修正しました:pray: |
203110f |
203110f
to
7d2682d
Compare
|
||
type Props = { | ||
children?: React.ReactNode; | ||
id?: string; |
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.
Q: こりは何用だったけっかな
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.
b6dd283
消し忘れでした:pray:
[K in Position]?: number; | ||
}; | ||
|
||
type Variant = { |
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.
今の使い方だとUnionでも行けそうな気もするけど、なにか理由あったっけかな。
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.
981e0f1
修正しました!
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.
LGTMです
おっ、testコケが |
z-index: -1; | ||
.spui-Toast-contentText { | ||
font-family: inherit; | ||
font-size: 14px; |
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.
Q: ここの絶対値は意図した記述ですかな
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.
43340ea
修正しました:pray:
デザインレビューいただいた部分含めて3つほどコミット追加しました
|
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.
挙動、ロジック問題なさそうです!
IMOで細かいところだけ挙げました(前回レビュー漏れですみません)🙏
d680965
to
6e11fd8
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.
errorのスタイルだけ微修正しました |
Formatが必要かも! (testコケ太郎) |
box-sizing: border-box; | ||
color: var(--color-text-accent-primary); | ||
cursor: default; |
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.
これは前からだたけど...default
指定しないと問題ありでしたっけかな
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.
これは、、おそらく不要なやつです、、
storybookでz-indexが効いていなかったので修正しました。合わせて説明も追記しました |
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.
23589bd
to
d1dd07a
Compare
BREAKING CHANGE: it changes some behaivior and adds new props
d1dd07a
to
45727db
Compare
Toastをアップデートし、以下の要素を追加しました。
variant="information"
の時は非表示できますinformation
,confirmation
,error
)TODO