-
Notifications
You must be signed in to change notification settings - Fork 10
fix: improve checkbox border styles and interaction overlay sizing #354
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
base: main
Are you sure you want to change the base?
Conversation
- Remove border when checkbox is checked or indeterminate - Add explicit borders for disabled, readonly, and invalid states - Fix interaction overlay sizing to use inherit instead of 100% 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 15bbda1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @MaxLee-dev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the visual presentation and behavior of the checkbox component. It streamlines the appearance of checked and indeterminate states by removing their borders, while simultaneously enhancing clarity by explicitly defining borders for disabled, readonly, and invalid states. Additionally, it addresses a sizing issue with the interaction overlay, ensuring it correctly adapts to its parent dimensions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
이번 PR은 체크박스의 상태별 테두리 스타일을 개선하고 상호작용 오버레이의 크기 문제를 수정한 점이 인상적입니다. 전반적으로 코드 품질을 높이는 좋은 변경이라고 생각합니다.
checkbox.css.ts 파일에서 disabled, readonly, invalid 상태의 테두리 두께를 1px로 지정하셨는데, 기존 베이스 스타일에 정의된 0.0625rem과 단위를 통일하면 코드의 일관성과 유지보수성을 더욱 높일 수 있을 것입니다. 이 부분에 대한 수정 제안을 리뷰 코멘트로 남겼습니다.
interactions.css.ts 파일에서 오버레이 크기를 100%에서 inherit으로 변경한 것은 부모 요소의 크기를 정확하게 따르도록 하는 올바른 수정으로 보입니다.
좋은 수정 감사합니다.
| '&[data-disabled]': { | ||
| border: `1px solid ${vars.color.border.normal}`, | ||
| opacity: 0.32, | ||
| pointerEvents: 'none', | ||
| }, | ||
|
|
||
| '&[data-readonly]': { backgroundColor: vars.color.gray['200'] }, | ||
| '&[data-readonly]': { | ||
| border: `1px solid ${vars.color.border.normal}`, | ||
| backgroundColor: vars.color.gray['200'], | ||
| }, | ||
| '&[data-readonly]:active::before': { opacity: 0.08 }, | ||
|
|
||
| '&[data-invalid]': { borderColor: vars.color.border.danger }, | ||
| '&[data-invalid]': { border: `1px solid ${vars.color.border.danger}` }, |
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.
일관성을 위해 1px 대신 베이스 스타일에 정의된 0.0625rem 단위를 사용하는 것이 좋겠습니다. 이렇게 하면 전체 컴포넌트에서 테두리 두께가 일관되게 유지되며, 루트 폰트 크기 변경에 따른 확장성도 확보할 수 있습니다. 1
| '&[data-disabled]': { | |
| border: `1px solid ${vars.color.border.normal}`, | |
| opacity: 0.32, | |
| pointerEvents: 'none', | |
| }, | |
| '&[data-readonly]': { backgroundColor: vars.color.gray['200'] }, | |
| '&[data-readonly]': { | |
| border: `1px solid ${vars.color.border.normal}`, | |
| backgroundColor: vars.color.gray['200'], | |
| }, | |
| '&[data-readonly]:active::before': { opacity: 0.08 }, | |
| '&[data-invalid]': { borderColor: vars.color.border.danger }, | |
| '&[data-invalid]': { border: `1px solid ${vars.color.border.danger}` }, | |
| '&[data-disabled]': { | |
| border: `0.0625rem solid ${vars.color.border.normal}`, | |
| opacity: 0.32, | |
| pointerEvents: 'none', | |
| }, | |
| '&[data-readonly]': { | |
| border: `0.0625rem solid ${vars.color.border.normal}`, | |
| backgroundColor: vars.color.gray['200'], | |
| }, | |
| '&[data-readonly]:active::before': { opacity: 0.08 }, | |
| '&[data-invalid]': { border: `0.0625rem solid ${vars.color.border.danger}` }, |
Style Guide References
Footnotes
-
디자인 토큰은 CSS 변수를 통해 참조하고 하드코딩을 지양해야 합니다. 여기서는
1px이라는 하드코딩된 값 대신, 베이스 스타일에 사용된0.0625rem과 같이 일관된 단위를 사용하거나 디자인 토큰 변수를 사용하는 것이 좋습니다. ↩
|
✅ All tests passed!
Click here if you need to update snapshots. |
| border: 'none', | ||
| }, | ||
|
|
||
| // NOTE: Prevents interaction styles from being applied when hovering over the label of a disabled radio button. | ||
| '&::before': { borderRadius: '0' }, | ||
|
|
||
| '&[data-disabled]::before': { opacity: 0 }, | ||
| '&[data-disabled]': { opacity: 0.32, pointerEvents: 'none' }, | ||
| '&[data-disabled]': { | ||
| border: `1px solid ${vars.color.border.normal}`, | ||
| opacity: 0.32, | ||
| pointerEvents: 'none', | ||
| }, | ||
|
|
||
| '&[data-readonly]': { backgroundColor: vars.color.gray['200'] }, | ||
| '&[data-readonly]': { | ||
| border: `1px solid ${vars.color.border.normal}`, | ||
| backgroundColor: vars.color.gray['200'], | ||
| }, |
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.
How about just changing borderWidth and borderColor separately, as defined in the base? Since borderStyle is a fixed value, there's no need to rewrite it every time!
+) Also, when disabled, there is no border.
'&[data-checked], &[data-indeterminate]': {
backgroundColor: vars.color.background.primary[200],
borderWidth: 0,
},
'&[data-readonly]': {
borderWidth: 1,
backgroundColor: vars.color.gray[200]
}
// ...| width: '100%', | ||
| height: '100%', | ||
| width: 'inherit', | ||
| height: 'inherit', |
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.
It breaks Popover, Button, and the other components' UI.
Summary
100%toinheritto properly respect parent dimensionsChanges
Checkbox Component (
checkbox.css.ts)border.normalcolorborder.normalcolorborderColorInteraction Mixin (
interactions.css.ts)::beforepseudo-element dimensions from100%toinheritfor better sizing behavior