-
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
Add elevation scale #64108
Add elevation scale #64108
Conversation
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: -301 B (-0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
A few thoughts off the top of my head:
|
Would it make sense to simply map each step in the scale to the
Are you asking which value would be applied to Popovers? Generally they contain un-interruptive configuration UI's, so
That's a good question. In terms of elevation the shadow styles will need to be different for light / dark modes. Darker colored themes will require darker shadow styles in order for them to be visible. I don't think radius should be affected, but another consideration is spacing; the theme component may need to support a 'density' option that affects spacing, type styles, components, etc. That part isn't really clear yet. |
That could work, although it could create unexpected changes in consumers. If we went down that way, we'd need to make sure to communicate it to consumers of those components via a dev note.
Absolutely. Changing the default popover box shadow is trivial, but what I really meant was that this is a great opportunity to review all popovers across Gutenberg and standardize their box shadow for better consistency and clarity. |
I think this would create discrepancies with the existing styles of |
Just capturing some small feedback/questions from here, relevant to the PR. |
Is this more of a system for elevation, or proposing what those elevations should be, or both? |
That could work, although the proposed naming convention might lead to confusion? Would it be strange if
The former, to avoid changing anything in production for now. We can define and apply values separately once we're happy with the scale. @jasmussen If we want fewer steps in the scale I'd be tempted to combine |
Seems fine with me. I'm always just in the category of start with as little as possible and then expand only when the need very clearly presents itself. |
I don't think it's currently needed because it doesn't fit the common logic of elevation (Level 0 and up) and the current design style (would be more skeuomorphic) but want to bring it up anyways, just to be sure: |
My thoughts were around using string |
@hanneslsm shadow styles are already being applied albeit sporadically without well defined intent. A big part of this effort is about defining how/when to use elevation as we embark on the broader admin design effort (#53322). Please refer to the four suggested steps in the scale which details this. In terms of mockups it's a little bit tricky because there are as yet undecided details to consider. For instance details like radius, color schemes, type, light/dark modes etc. With that said consider the following rough in terms of those details (including the shadows); they're primarily to demonstrate the 4 steps in the scale in some existing flows and experiences.
|
Thanks for clarifying! Looks good! |
I don't think there's a huge amount in it, but I've switched the names to match the radius variables, IE to use
I also added some examples of where the different values would be applied across the existing set of components.
|
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.
This looks correct to me from a technical standpoint. The only concern I have is that the naming might be confusing since it doesn't contain "shadow", yet it's supposed to be an option for shadow property values. However, it's not a big deal so not going to block on that.
Appreciate the comments with added context on usage of each option, it's a nice plus.
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.
Ah, missed that this is currently broken because $shadow-popover
is assigned $elevation-10
(similar story with "modal"), guess it's just out of sync after recent changes in naming. After this is corrected, my previous (approval) review stands. Example failure in CI: https://github.com/WordPress/gutenberg/actions/runs/10249250569/job/28352231809?pr=64108
Good spot, I fixed that. |
@DaniGuardiola @ciampo do you think this needs a components changelog entry? |
@jameskoster good question. @ciampo and I discussed and we both think it's not necessary. |
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.
Issue was addressed 👍
@@ -74,6 +74,10 @@ export default Object.assign( {}, CONTROL_PROPS, TOGGLE_GROUP_CONTROL_PROPS, { | |||
cardPaddingMedium: `${ space( 4 ) } ${ space( 6 ) }`, | |||
cardPaddingLarge: `${ space( 6 ) } ${ space( 8 ) }`, | |||
popoverShadow: `0 0.7px 1px rgba(0, 0, 0, 0.1), 0 1.2px 1.7px -0.2px rgba(0, 0, 0, 0.1), 0 2.3px 3.3px -0.5px rgba(0, 0, 0, 0.1)`, |
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.
Since I don't think that popoverShadow
is exported from the package, I think it should be OK to remove it and replace its usages with elevationXSmall
.
This can be definitely done in one of the follow-up PRs
Cool. I'll go ahead and merge this. We can continue to refine the scale and values as needed when we begin applying across components. |
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: hanneslsm <hanneslsm@git.wordpress.org>
This PR adds variables for the elevation scale as outlined in #63757. It follows the method applied in #64007 to add the radius scale.
$shadow-popover
and$shadow-modal
) by mapping them to the new onesElevation scale
There are four values in the scale:
$elevation-x-small
: For sections and containers that group related content and controls, which may overlap other content. Example: Preview Frame.$elevation-small
: For components that provide contextual feedback without being intrusive. Generally non-interruptive. Example: Tooltips, Snackbar.$elevation-medium
: For components that offer additional actions. Example: Menus, Command Palette$elevation-large
: For components that confirm decisions or handle necessary interruptions. Example: Modals.In terms of values, I used the existing
$shadow-popover
for$elevation-x-small
, and$shadow-modal
for$elevation-large
.$elevation-small
and$elevation-medium
are new values that I created by hand to slot between the existing values. It's possible these will change when it comes to applying the new scale across components.Application
$elevation-x-small
$elevation-small
$elevation-medium
elevation-large
ZStack
Potential input elements such as; Range handle, Active item in ToggleGroupControl, Toggle handle
Resize handle
Block toolbar
Tooltip
Popover
Command Palette
ConfirmDialog
Guide