feat(react): update radio v4 #522
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -9.33 kB (-0.3%) Total Size: 3.12 MB
ℹ️ View Unchanged
|
|
Visit the preview URL for this PR (updated for commit a4414d6): https://pixiv-charcoal-web--pr522-naporitan-add-radio-96nmpl80.web.app (expires Wed, 29 May 2024 05:34:43 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 314b26d3adca98a761c7e4d9922ebb206ff024a0 |
naporin0624
added
the
image snapshots update
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
commented
Apr 24, 2024
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
commented
Apr 24, 2024
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
force-pushed
the
naporitan/add-radio-v4
branch
from
372c6a5 to
be0023c
Compare
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
This reverts commit be0023c.
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
removed
the
image snapshots update
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
added
image snapshots update
toshusai
approved these changes
May 16, 2024
toshusai
reviewed
May 16, 2024
| @@ -23,6 +24,8 @@ const Radio = forwardRef<HTMLInputElement, RadioProps>(function RadioInner( | |||
| onChange, | |||
| } = useContext(RadioGroupContext) | |||
|
|
|||
| const className = useClassNames('charcoal-radio', props.className) | |||
There was a problem hiding this comment.
I think it's better to use names that include DOM tags.
charcoal-radio=>charcoal-radio__labelcharcoal-radio__label=>charcoal-radio__label_div
There was a problem hiding this comment.
ok thank you review.
toshusai
reviewed
May 16, 2024
Comment on lines
93
to
106
| .charcoal-radio__label::before { | ||
| display: block; | ||
| width: 0; | ||
| height: 0; | ||
| content: ''; | ||
| margin-top: -4px; | ||
| } | ||
| .charcoal-radio__label::after { | ||
| display: block; | ||
| width: 0; | ||
| height: 0; | ||
| content: ''; | ||
| margin-bottom: -4px; | ||
| } |
There was a problem hiding this comment.
We can remove these styles. The new style doesn't use any hacked adjustments.
There was a problem hiding this comment.
Oh. Ok!
'll fix the other PRs.
| .charcoal-radio__label { | ||
| font-size: 14px; | ||
| line-height: 22px; | ||
| display: flow-root; |
There was a problem hiding this comment.
We can remove display: flow-root;
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
toshusai
reviewed
May 16, 2024
| @@ -211,43 +99,46 @@ const RadioGroupContext = React.createContext<RadioGroupContext>({ | |||
| }) | |||
|
|
|||
| export function RadioGroup<Value extends string = string>({ | |||
There was a problem hiding this comment.
This might be a good opportunity to switch to forwardRef. In addition, make sure to pass the rest props to the div.
| @@ -11,7 +12,7 @@ export type RadioProps = React.PropsWithChildren<{ | |||
| }> | |||
|
|
|||
| const Radio = forwardRef<HTMLInputElement, RadioProps>(function RadioInner( | |||
| { value, disabled = false, children, className }, | |||
| { value, disabled = false, children, ...props }, | |||
There was a problem hiding this comment.
There is the same issue here.
https://github.com/pixiv/charcoal/pull/523/files#r1602999626
naporin0624
added
the
image snapshots update
toshusai
approved these changes
May 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
やったこと
動作確認環境
チェックリスト
不要なチェック項目は消して構いません
備考
@react-aria/radioがあるがそちらを使ったほうが良いか？