-
Notifications
You must be signed in to change notification settings - Fork 38
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
fix: update design token usage #652
Conversation
Visit the preview URL for this PR (updated for commit df4f644): https://pixiv-charcoal-web--pr652-yue-replace-token1-m-g5770as6.web.app (expires Wed, 27 Nov 2024 06:42:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 314b26d3adca98a761c7e4d9922ebb206ff024a0 |
Size Change: 0 B Total Size: 540 kB ℹ️ View Unchanged
|
d413775
to
371efbc
Compare
665ba02
to
e26d751
Compare
render: () => ( | ||
<Clickable | ||
style={{ | ||
color: 'var(--charcoal-color-text-secondary-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.
Clickable自体はinherentで問題がないがstroybookのためtext-secondaryにする
placeholder="Placeholder" | ||
label="Label" |
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.
移行時たりてなかったようでした
render: function Render(args) { | ||
const [value, setValue] = useState<Option>(options[0]) | ||
|
||
const id = useId() |
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が同じページに2回レンダリングされるので、別のidにしないと壊れます
3ad21b8
to
5aff40b
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.
修正ありがとうございます!
いくつか漏れがあったようでそこも直していただきありがとうございました:bow:
一点コメントしましたが、外部から children がさされない環境なので問題ないものなので approve します!
|
||
.charcoal-checkbox-input:checked:not(:disabled):hover { | ||
.charcoal-checkbox-input:checked:not(:disabled, :read-only):hover { |
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.
NITS
明示的に readonly 属性がついているときのみ動作する [readonly]
selector のほうが好ましいのかなと思いました
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.
[readonly]
にすると <input readonly="false"/>
が選択されそうな気がしたけどHTML的にそもそも不正でreadonly扱いしたね。
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.
opacity: 0.32; | ||
} | ||
|
||
.charcoal-checkbox__label:has(input:read-only) { |
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.
NITS
明示的に readonly 属性がついているときのみ動作する [readonly] selector のほうが好ましいのかなと思いました
やったこと
動作確認環境
チェックリスト
不要なチェック項目は消して構いません