Update components radius #64368
Merged
Update components radius #64368
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: -48 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
tyxla
reviewed
Aug 12, 2024
There was a problem hiding this comment.
Thanks for working on it @jameskoster 🙌
This mostly looks good in my testing ✅
I've asked a few questions about some of the changes - let me know what you think.
Looks like you'll need to update a bunch of test snapshots. Happy to help with that once you confirm all changes are expected.
Finally, I'd definitely add changelog entries here. Perhaps it makes sense to list all affected components.
| @@ -47,7 +47,7 @@ $radius-x-small: 1px; // Applied to elements like buttons nested within primit | |||
| $radius-small: 2px; // Applied to most primitives. | |||
| $radius-medium: 4px; // Applied to containers with smaller padding. | |||
| $radius-large: 8px; // Applied to containers with larger padding. | |||
| $radius-full: 9999px; // For lozenges. | |||
| $radius-full: 9999px; // For pills. | |||
There was a problem hiding this comment.
👍 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 than lozenge.
| @@ -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.
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 radiusSmall or radiusMedium?
There was a problem hiding this comment.
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 radiusXSmall (2px) is technically adequate. However if we ever decide to increase the height of the progress bar then the track/indicator would no longer be fully rounded. But utilising radiusFull we ensure the bar is fully rounded regardless of how tall it is.
There was a problem hiding this comment.
Since it is only 2px tall radiusXSmall (2px) is technically adequate.
I assume you meant radiusSmall (2px) since radiusXSmall is 1px.
Anyway, that's not a problem for your intent here, and making it rounded makes sense to me 👍
| @@ -58,7 +58,7 @@ export const Indicator = styled.div< { | |||
| position: absolute; | |||
| top: 0; | |||
| height: 100%; | |||
| border-radius: ${ CONFIG.radiusBlockUi }; | |||
| border-radius: ${ CONFIG.radiusFull }; | |||
| @@ -9,7 +9,6 @@ | |||
| .components-modal__content { | |||
| padding: 0; | |||
| margin-top: 0; | |||
| border-radius: $radius-block-ui; | |||
There was a problem hiding this comment.
Agree to keep this consistent with the dialog/modal component we use 👍
| 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.
Did you mean to increase this to 4px? I don't see it in the list of intended changes.
There was a problem hiding this comment.
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.
That's correct. But why do we even need the border-radius then? It feels like it's redundant.
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.
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.
I don't mind keeping it if that's what you prefer 👍
| @@ -1,7 +1,6 @@ | |||
| .components-toolbar-group { | |||
| min-height: $block-toolbar-height; | |||
| border-right: $border-width solid $gray-900; | |||
| background-color: $white; | |||
There was a problem hiding this comment.
This feels like something unintended. Should it be part of another PR? I know we're overriding the background color here and there, so we might want to audit those instances separately, for example:
There was a problem hiding this comment.
The change is intended; without it, the group background obscures the radius of the container:
We could add a radius to the group instead of removing the background, but that causes issues with the separator styling:
I don't mind extracting the toolbar change from this PR and updating it separately though. In fact that's probably a good idea given it's likely the most impactful change. We can discuss the intricacies in a different PR.
There was a problem hiding this comment.
Yeah, I'd prefer a separate PR if that's possible. But I'll leave that call to you, it doesn't feel like this is a blocker for this PR anyway.
jameskoster
commented
Aug 12, 2024
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
tyxla
approved these changes
Aug 13, 2024
There was a problem hiding this comment.
LGTM 👍 Thanks @jameskoster 🙌
Part of it can be split out to another PR, as discussed above, but that's not a blocker.
CHANGELOG will need a rebase before 🚢 .
Co-authored-by: Marin Atanasov <[email protected]>
|
Thanks for the review @tyxla :) I'd appreciate if someone from @WordPress/gutenberg-design could take a look when they get a second. |
|
We can try it! |
ciampo
reviewed
Aug 14, 2024
ciampo
reviewed
Aug 14, 2024
There was a problem hiding this comment.
Thank you for working on this, @jameskoster !
I think we should be able to remove the radiusBlockUi variable from the internal component config !
I also see snapshot tests failing — while we update them, let's take it as one additional way to make sure that there are no unintended changes in this PR.
|
Flaky tests detected in 92fbdbb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10407238926
|
ciampo
approved these changes
Aug 15, 2024
There was a problem hiding this comment.
Changes are looking good!
We also have a few usages left of the controlBorderRadius shared variable (in PaletteEdit and ItemGroup) — feel free to address them here or in a follow-up PR. Either way, we should then be able to remove controlBorderRadius too from the shared config!
|
@jameskoster Looking good! We can go ahead and merge whenever you're ready to :) |
artemiomorales
added
[Feature] UI Components
This was referenced Aug 30, 2024
Merged
2 tasks
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.
Most of the changes here do not lead to any visual differences. Those that do are a consequence of adopting the expanded radius scale and noted below. As a reminder here's the current scale and guidelines:
$radius-x-small:Applied to elements like buttons nested within primitives like inputs.$radius-small: Applied to most primitives.$radius-medium: Applied to containers with smaller padding.$radius-large: Applied to containers with larger padding.$radius-full: For pills.$radius-round: For circles and ovals.Color palette
The color card in the picker has increased radius.
Guide
The modal has increased radius. Arguably this should be inherited from the main
Modalcomponent—how do y'all feel about making that change here?Modal
Increased radius on outer container.
Placeholder
Increased radius on outer container.
Note
This change is only visible in Storybook. It seems that block placeholders override this, so there will be no visual change in-product.
Popover
Increased radius on outer container.
Snackbar
Increased radius on outer container.
Toggle Group Control
Decreased radius on inner buttons.
Unit control
Decreased radius on inner button.