Update Card radius #65053
Merged
Update Card radius #65053
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
[Type] Enhancement
|
Size Change: +79 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Sep 4, 2024
ciampo
reviewed
Sep 4, 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. |
|
For the style card use case, 8px feels a bit large doesn't it? Wouldn't 4px be more right in this case? I.e. wouldn't that card eventually be shown in context of other 4px radii? |
|
Conceptually Cards should be used to provide structure in more complex views by grouping related UI together in order to aid visual scanning. Imagine a settings page with several sections/sub-sections. Generally a Card would be quite a large container which suggests the large radius is warranted, according to the scale. I agree in the case of the style tile that Alternatively, if we don't want to add any interactivity it could just be a plain container, not a Card. Finally, if we want to continue using Card but don't want the roundness, we could just apply |
|
I'm biased against cards, and in favor of spacing and borders to place out things instead. But since the component exists, absolutely important to make it shine, potentially even elevation can play a role in the card, i.e. if it's elevation 0 it doesn't have a border? About the radius, though, would you ever use a card inside a modal? If yes, then the radius should be smaller than the modal no? If you feel strongly about the 8px radius, I wouldn't mind it, though I would find a different component to use to wrap the style card, so that maintains the same radius as its immediate surroundings. |
@jameskoster , can you reproduce this behaviour in Storybook? If not, it may be something related to a specific use of I see that we are discussing
|
It's hard to say for sure, but my instinct is no. Modals/Dialogs should generally be concise interfaces that do not require the additional structure that cards provide. If a designer ends up using a Card inside a Modal then I'd be inclined to question whether a Modal is the right interface. One good thing is that Card isn't used much currently, and this is a tiny visual change in the grand scheme, so we can easily revisit later if/when consumption sees an uptick and we have a better idea about how it's being used. @ciampo these are all good considerations. I agree the design/implementation could be revisited, but until there's an immediate need it's unlikely to be prioritised. This PR is mostly about updating the current implementation to adopt the design system fundamentals. As I see it the path forward could be:
If we want to avoid any temporary visual diffs then step 2 should happen in this PR, or a separate one that is merged before this one. Any strong feelings there? All that said, I don't see this as a huge priority. If we'd rather leave it as-is for now and update the entire component later that also works for me. |
|
Sounds good, definitely a good idea to work on this iteratively and when there's a higher priority. I was just highlighting that there's an opportunity to make changes to the components while we're taking a look at the design system. Once there's agreement on the actual border radius value, we can merge an iterate as appropriate 👌 |
|
To unblock this I've pushed a couple of small overrides in the site editor to keep the global styles UI closer to trunk.
I still think we should revisit using |
ciampo
approved these changes
Sep 10, 2024
| @@ -221,3 +221,7 @@ | |||
| fill: currentColor; | |||
| } | |||
|
|
|||
| .edit-site-global-styles-screen-root__active-style-tile, | |||
| .edit-site-global-styles-screen-root__active-style-tile-preview { | |||
| border-radius: $radius-small !important; // Todo: revisit use of Card component and/or remove override | |||
There was a problem hiding this comment.
is the !important necessary?
There was a problem hiding this comment.
The component styles are added inline, so unless there's a better way to perform the override (?) I think it is necessary.
There was a problem hiding this comment.
I don't see inline styles on those components — we can still apply overrides with a little specificity hack that is not as invasive as !important:
.edit-site-global-styles-screen-root__active-style-tile {
// Todo: revisit use of Card component and/or remove override
// The &#{&} is a workaround for the specificity of the Card component.
&#{&},
&#{&} .edit-site-global-styles-screen-root__active-style-tile-preview {
border-radius: $radius-small;
}
}There was a problem hiding this comment.
Sorry I meant added by js (I think). TIL there's a way to override that :)
There was a problem hiding this comment.
Yup! It's basically repeating the classname selector, ie. .edit-site-global-styles-screen-root__active-style-tile.edit-site-global-styles-screen-root__active-style-tile
|
What do you think @jasmussen ? |
|
Sorry I missed the pings! I've longer term questions about the use of the card component itself, but none that are at all blocking to this work. Continue apace! (Knowing that the Style card was updated to not have the larger radius.) |
|
@ciampo I think the snapshots need updating here. Would you mind handling that, or letting me know the command to do it myself? :D |
ciampo
force-pushed
the
update/card-radius
branch
from
0fdbfe8
to
cb55be0
Compare
|
Sure! I rebased and pushed a commit to update the snapshots FYI, |
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?
Update the border radius applied to the
Cardcomponent.Why?
#64368 sought to update all components, but Card was missed. This PR rectifies that.
radiusLargeis applied as this aligns with the guidelines in the radius scale:I think we can consider
Cardsimilar toModalin this respect.Visual changes in the editor
There are very few consumers of this component so visual changes are minimal. The main one seems to be the variation tile in global styles.
I'm not entirely sure the use of
Cardis warranted in this case, so this might be a detail to revisit separately.Testing Instructions
npm run storybook:dev