Colorize template parts and Reusable blocks #45473
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
|
|
glendaviesnz
commented
Nov 2, 2022
| @@ -24,3 +24,8 @@ $light-gray-placeholder: rgba($white, 0.65); | |||
| $alert-yellow: #f0b849; | |||
| $alert-red: #cc1818; | |||
| $alert-green: #4ab866; | |||
|
|
|||
There was a problem hiding this comment.
not sure if this is the best place to put these - went with CSS vars instead of SCSS vars so themes can potentially override these.
There was a problem hiding this comment.
The choice of CSS vars over SCSS makes sense to me 👍
|
Size Change: +12.1 kB (+1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
talldan
reviewed
Nov 2, 2022
Comment on lines
77
to
79
| const isReusable = | ||
| blockType.name === 'core/block' && blockType.category === 'reusable'; | ||
| const isTemplatePart = blockType.name === 'core/template-part'; |
There was a problem hiding this comment.
There are some functions isTemplatePart and isReusableBlock in the blocks package that could be useful for these checks.
There was a problem hiding this comment.
ah, perfect, thanks, will switch to that
|
It's very colorful! I think the approach taken here is probably the right one for the first version (vs. some configuration on the block). May need to be a bit careful about meeting contrast criteria for text against the blue background (e.g. for Uncle Bob in the video): I haven't checked these, but my hunch is the greeny-blue text color may not pass. |
|
Fast work, checks the boxes, nice work! Here's a GIF showing the list view with purple for template parts, green for reusable blocks:
I know @jameskoster did a lot of work on this, including finding the right colors. Would love a sanity check here. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Joen and I huddled on this one a bit and came up with the following:
If we update the PR to reflect the design above I think we'll be in a good spot :) |
Looks good to me as well. |
|
Thanks for the updated designs @jasmussen & @jameskoster - I have added a todo list to the PR description, got a couple of things done and 🤞 can get the bulk of the rest done tomorrow, will let you know when it is ready for another review. |
|
@jameskoster with the template part name in the site editor header bar, the last designs show the template icon there as well as the text colored. This bar doesn't currently show the icon, do you want this added? |
|
I think all that is left is to color the children and background in the list view, which proved a little twickier than expected. I am away from the keyboard all next week so may not get back to this until week of 14 November - if someone feels like finishing off that part before then, go for it. |
|
Yes I think it would be good to try adding the icon, it can be useful in cases where the name doesn't denote the semantic purpose. I noticed the design we shared broke the rule of colorising the text though. Here's an updated version that uses the same background color that we want to employ in list view:
On that note, it would be good to align these "light" color variations with the recent update to hover styles in the Inserter:
This is |
|
Agreed on settling on a single color for anything reusable / synced. @jameskoster note the hover effect on the inserter (and on selected nav items in the pattern inserter) is actually using |
mtias
added
[Feature] Synced Patterns
glendaviesnz
force-pushed
the
add/color-to-template-parts
branch
from
3fc3b80
to
e8ec5e6
Compare
|
@jameskoster I think it is all covered now, let me know if I have missed anything. |
|
Thanks @glendaviesnz, it's looking good, everything from the design appears to be accounted for 🎉 One thing I'd like a sanity check on before approving is the treatment of child blocks within synced blocks. I know we stipulated the purple icon in the design, but for some reason it tripped me up a little when I built the PR. I'm wondering if the purple background alone is adequate, and whether coloring the icon as well might mislead folks into thinking the child blocks are synced blocks in their own right? 🤔 What do you think @WordPress/gutenberg-design ? |
|
I was going to comment on that. Since we can have nested synced blocks, it'd be better to not colorize the icon of the children, in my opinion, but we can try what feels correct in use and testing |
|
Yeah good point about the nesting of synced blocks. If all icons are purple you can't see at a glance which ones are synced. @glendaviesnz let's remove the colorisation of the child blocks, unless they are synced of course :)
|
Yeh, I wasn't sure about that, my thinking was that if a block was a child of a template part or reusable block it is essentially 'synced', ie. changes to it are reflected in all places, but will remove it as suggested. |
This reverts commit 5ff8bd6.
glendaviesnz
force-pushed
the
add/color-to-template-parts
branch
from
d9fa111
to
2333a27
Compare
ntsekouras
reviewed
Nov 23, 2022
| @@ -17,6 +22,7 @@ function BlockCard( { | |||
| blockType, | |||
| parentBlockClientId, | |||
| handleBackButton, | |||
| isSynced, | |||
There was a problem hiding this comment.
I think it would make better to add className here instead of this specific prop. It would save us possible deprecations and we could also accommodate more use case for styles in the future. What do you think?
There was a problem hiding this comment.
Good idea - I will take a look at that tomorrow.
16 tasks
|
Should this be displayed on the frontend for some reason?
|
|
@gyurmey2 thanks for reporting, they shouldn't be duplicated like that - a PR to hopefully fix this is here. |
|
@gyurmey2 the fix for this has been merged |
38 tasks
This was referenced Mar 1, 2023
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 color to template parts and reusable blocks in list view, block toolbar, and block selection outlines
Why?
To help differentiate these block types when editing.
Fixes: #32163
How?
A few extra classes, a bit of CSS, and Bob's your uncle
Todo
Testing Instructions
Screenshots or screencast
colorize.mp4