Tag Cloud Block: Add outline style #37092
Merged
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: +426 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
mtias
reviewed
Dec 3, 2021
|
|
||
| &.is-style-pill a { | ||
| border: 1px solid var(--wp--preset--color--foreground); | ||
| border-radius: 5px; |
There was a problem hiding this comment.
We could make it a block attribute, but it wouldn't do anything unless this style was used.
There was a problem hiding this comment.
It should only show up if the style is being used. We can also not set it to anything and let themes customize it for now.
There was a problem hiding this comment.
Using a custom variable in theme.json?
There was a problem hiding this comment.
No, the theme can just target it with CSS directly if they want to
scruffian
commented
Dec 3, 2021
| border: 1px solid var(--wp--preset--color--foreground); | ||
| border-radius: 5px; | ||
| font-size: var(--wp--preset--font-size--normal); | ||
| margin-bottom: 10px; |
There was a problem hiding this comment.
We might want to use some factor of block gap here.
There was a problem hiding this comment.
Can it be a percentage so that if font-size is changed it works? Same for padding.
There was a problem hiding this comment.
I don't think a percentage will work, but I updated it to use ch units which does. See the update screenshots above...
scruffian
commented
Dec 3, 2021
| @@ -18,4 +18,18 @@ | |||
| margin-left: 5px; | |||
| text-decoration: none; | |||
| } | |||
|
|
|||
| &.is-style-pill a { | |||
There was a problem hiding this comment.
Should we remove the underline in this case?
There was a problem hiding this comment.
Yeah, I don't think an underline is necessary.
kjellr
reviewed
Dec 7, 2021
| &:active, | ||
| &:hover { | ||
| background-color: var(--wp--preset--color--foreground); | ||
| color: var(--wp--preset--color--background); |
There was a problem hiding this comment.
These variables don't exist by default, unfortunately.
I'm not sure that we can specify a nice reversed-out hover state without tackling WordPress/twentytwentytwo#99 first.
There was a problem hiding this comment.
I think we can avoid hover effects here
|
I like this — it's a more modern version of a tag cloud. I think the only issue with it is figuring out how it could be implemented in a way that's lightweight and flexible enough to work out of the box for many themes. The current approach feels a bit too hardcoded to me. |
scruffian
force-pushed
the
add/pill-style
branch
from
b69a1d0 to
386c9e5
Compare
kjellr
reviewed
Dec 16, 2021
|
|
||
| &.is-style-pill a { | ||
| border: 1px solid currentColor; | ||
| font-size: var(--wp--preset--font-size--normal) !important; // !important Needed to override the inline styles. |
There was a problem hiding this comment.
The normal font size has been depreciated, so we might want to switch to "Medium" here?
.... but if we don't specify a font size maybe we're better off? Then it should inherit the default size.
There was a problem hiding this comment.
The problem is if we don't specify a size then the font sizes are determined by how often the tag is used
There was a problem hiding this comment.
unset will do that for you
|
I'm trying this on emptytheme and the frontend doesn't reflect what's in the editor: Editor:
Frontend:
|
|
It works for me in emptytheme: |
|
It's looking right in the front end for me. 👍 In general, this seems solid to me. The code is generic enough that it inherits all it can from the theme its placed in. Nice work! The only remaining comment on my end is that I think we should reconsider the "pill" name since these aren't rounded anymore. Maybe we could use "Button" or "Outline" instead? Outline might be more reasonable, since folks would assume a "Button" would match the button block. |
scruffian
changed the title
Done! |
kjellr
approved these changes
Jan 4, 2022
|
It would be good to have another review before we merge so this doesn't end up needing a revert. |
|
I have no objections here as long as designers are happy |
gziolo
reviewed
Jan 21, 2022
| @@ -51,5 +51,14 @@ function register_block_core_tag_cloud() { | |||
| 'render_callback' => 'render_block_core_tag_cloud', | |||
| ) | |||
| ); | |||
|
|
|||
| register_block_style( | |||
There was a problem hiding this comment.
Any particular reason why this style isn't included in the block.json file? See: https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#block-styles.
gziolo
reviewed
Jan 21, 2022
| array( | ||
| 'name' => 'outline', | ||
| 'label' => __( 'Outline', 'gutenberg' ), | ||
| 'style_handle' => 'outline', |
There was a problem hiding this comment.
I don't see a style registered with the handle outline in this PR and the styles were added to the existing CSS file, so maybe we don't need that part.
8 tasks
7 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.
Description
This adds an outline style to the Tag Cloud block. We don't yet have an agreed standard for color names, so it's possible that these presets won't be defined in the theme.
How has this been tested?
In TT2 and Skatepark
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).