-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update components radius #64368
Update components radius #64368
Changes from 8 commits
d8ba1b1
d80f545
f19feb0
1dd8287
935ffd0
3c2a4cf
460266f
9378eef
f6b97bf
83380d0
80a91d8
161854e
92fbdbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,15 +7,15 @@ import { css } from '@emotion/react'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { COLORS } from '../../utils'; | ||
import { COLORS, CONFIG } from '../../utils'; | ||
import type { | ||
AlignmentMatrixControlProps, | ||
AlignmentMatrixControlCellProps, | ||
} from '../types'; | ||
|
||
export const rootBase = () => { | ||
return css` | ||
border-radius: 2px; | ||
border-radius: ${ CONFIG.radiusMedium }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean to increase this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My feeling is this falls into 'small container' territory. That said, I don't think the radius of this component is visually rendered anywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. But why do we even need the If it's there just for the case when someone provides a custom background color, then why can't the consumer also add a border-radius when they need it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but it seems safer to adjust the value here rather than remove it? We could remove it in a follow up with a dedicated changelog entry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind keeping it if that's what you prefer 👍 |
||
box-sizing: border-box; | ||
direction: ltr; | ||
display: grid; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ | |
.components-modal__content { | ||
padding: 0; | ||
margin-top: 0; | ||
border-radius: $radius-block-ui; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree to keep this consistent with the dialog/modal component we use 👍 |
||
|
||
&::before { | ||
content: none; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ export const Track = styled.div` | |
${ COLORS.theme.foreground }, | ||
transparent 90% | ||
); | ||
border-radius: ${ CONFIG.radiusBlockUi }; | ||
border-radius: ${ CONFIG.radiusFull }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This used to be 2px and is now 9999px. I also don't see this in the list of changes in the PR description. Did you mean to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. Visually there's no change here which is why I didn't list it. The design of the progress bar calls for a fully rounded track/indicator. Since it is only 2px tall There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I assume you meant Anyway, that's not a problem for your intent here, and making it rounded makes sense to me 👍 |
||
|
||
// Windows high contrast mode. | ||
outline: 2px solid transparent; | ||
|
@@ -58,7 +58,7 @@ export const Indicator = styled.div< { | |
position: absolute; | ||
top: 0; | ||
height: 100%; | ||
border-radius: ${ CONFIG.radiusBlockUi }; | ||
border-radius: ${ CONFIG.radiusFull }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
/* Text color at 90% opacity */ | ||
background-color: color-mix( | ||
in srgb, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ | |
|
||
&::before { | ||
content: ""; | ||
border-radius: 50%; | ||
border-radius: $radius-round; | ||
} | ||
} | ||
} | ||
|
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.
👍 Thank you for this. As a non-native speaker, the difference between tablet, pill, lozenge, pellet, capsule, etc. makes my head hurt.
pill
does like a word that's more common thanlozenge
.