Add radius scale #64007
Merged
Add radius scale #64007
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
jameskoster
added
Needs Design Feedback
|
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. |
|
Size Change: +1.99 kB (+0.11%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Yes, that would be helpful! |
Considering that we have in variable.scss already (That's also how it's done in TT4 for font sizes) |
tyxla
reviewed
Jul 29, 2024
packages/base-styles/_variables.scss
Outdated
| @@ -91,7 +102,6 @@ $border-width: 1px; | |||
| $border-width-focus-fallback: 2px; // This exists as a fallback, and is ideally overridden by var(--wp-admin-border-width-focus) unless in some SASS math cases. | |||
| $border-width-tab: 1.5px; | |||
| $helptext-font-size: 12px; | |||
| $radius-round: 50%; | |||
| $radius-block-ui: 2px; | |||
There was a problem hiding this comment.
Should $radius-s replace the $radius-block-ui or be part of its declaration?
There was a problem hiding this comment.
I'd welcome thoughts on that. My idea was to essentially deprecate $radius-block-ui by assigning it the radius-s value like you suggest, then updating all the styles that utilises it. That second step is a big one though.
There was a problem hiding this comment.
With the radius scale you're proposing, $radius-block-ui becomes even more confusing to have in the first place. But we can always add a TODO comment and refactor away from it in a simultaneous PR, in favor of its radius-s counterpart.
There was a problem hiding this comment.
becomes even more confusing to have in the first place
I agree, but I assume it would not be safe to remove?
There was a problem hiding this comment.
No, removing it will be unsafe and could break potential users, but we can:
- Update
$radius-block-uito use$radius-smallinstead of a hardcoded value - Update the entire codebase to use
$radius-smallinstead. - Deprecate (through a comment, similar to others in this file)
$radius-block-uiso it's clearer that it shouldn't be used.
There was a problem hiding this comment.
The safest thing would be to keep it around, aliasing it to $radius-small. We could then also add a deprecation notice (I don't know how those work for Sass variables), and maybe a stylelint rule to prevent further usages.
Not sure if we'd be able to remove it entirely, since a missing Sass variable would cause build errors. So, on one end it would be easy to spot and fix (especially if we add a dev note), on the other end I'm not sure we're ok with causing build errors.
There was a problem hiding this comment.
Sounds like a plan :)
There was a problem hiding this comment.
I pushed a commit to update/deprecate $block-radius-ui: ea32b33. I don't mind doing that in a separate PR if y'all prefer though.
Good point. Edit: |
I'd prefer |
|
Agreed with the points made above. In general, I'd go for consistency with how existing variables are named. |
ciampo
approved these changes
Jul 30, 2024
There was a problem hiding this comment.
Current set of changes LGTM 🚀
As for the next steps:
- we should open a PR where we remove all usages of
$radius-block-uiin favour of the correct$radius-*small; - we should then open another PR where we add a deprecation notice (if that's a think that can be done in Sass), and also a stylelint rule to prevent further usages of the deprecated variable
- we should add a dev note (maybe to the parent issue?) to make sure that third-party developers are aware of the deprecation and how to correctly update their code
|
I'll work on that first step, but might need some assistance with the others. |
|
Sounds good, feel free to ping us to review and help with follow-up tasks :) |
Merged
|
I will also mention that we have a |
jameskoster
added
the
Design System
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.
What?
Adds variables for the radius scale as described in #63703.
Guidelines
$radius-x-small: The smallest radius. Applied to contained elements within primitives like inputs or buttons. E.g. the selected item in aToggleGroupControl, or the unit selector inUnitControl.$radius-small: Applied to primitive components like Badges, Buttons, Inputs, Checkboxes.$radius-medium: Applied to containers with smaller padding, e.g. Menus, Notices, Snackbars, Block toolbar. Also used for focus rings on primitive components.$radius-large: Applied to containers with larger padding, e.g. Modals, Pages, and the Preview Frame.$radius-full: For creating lozenges.$radius-round: For creating circles or ovals. (This variable already exists).Why?
Utilising a consistent scale across will add polish to the UI and reduce maintenance overheads. The diagram below demonstrates how elements would nest neatly inside one another once the scale has been adopted, from the individual button in a
ToggleGroupControlup to the outer container. The visuals are intentionally abstract to draw focus onto the radius, not the overall appearance.The values could change when they're applied (in a separate effort). For now they are based on the existing convention which is a
2pxradius applied to virtually everything. For this PR the important detail to concentrate on is the scale itself._variables.scss?