Update site editor block placeholder styling #35513
Conversation
jameskoster
added
the
[Feature] Site Editor
|
Size Change: -445 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
| @@ -39,13 +39,11 @@ | |||
| @import "./template-part/editor.scss"; | |||
| @import "./text-columns/editor.scss"; | |||
| @import "./video/editor.scss"; | |||
| @import "./query-title/editor.scss"; | |||
There was a problem hiding this comment.
I love that we can remove all of these single-purpose stylesheets.
| <Warning> | ||
| { __( 'Post excerpt block: no post found.' ) } | ||
| </Warning> | ||
| { __( 'Post excerpt block: no post found.' ) } |
There was a problem hiding this comment.
This seems good enough to me. What did you think of just showing the block name?
That said, I'm losing a bit of orientation when using this one right now:
There was a problem hiding this comment.
So this occurs when you're editing content in the site editor, and the content that you're editing has no excerpt, but the template attempts to display it.
I just checked and it turns out this issue exists on trunk. I think it's a separate thing, so we can probably create a dedicated issue.
What did you think of just showing the block name?
Totally, I was going to do the content stuff in a separate PR, but I suppose we could do it here.
| <Icon icon={ postFeaturedImage } /> | ||
| <p> { __( 'Featured Image' ) }</p> | ||
| </div> | ||
| <Placeholder className={ 'block-editor-media-placeholder' }> |
There was a problem hiding this comment.
I'm impressed you took this one on directly! I do think maybe this one might be better as a separate PR, as it's likely more of a mouthful than the first glance. For example, it's not looking too great in the post editor:
There's a discussion on what role it serves in the post editor, but nevertheless it is there at the moment.
I also think we'll either want to tweak the mountainrange illustration, or make it scale to fill the entire canvas rather than just be centered in the middle:
| @@ -6,29 +6,55 @@ div[data-type="core/post-featured-image"] { | |||
| } | |||
| } | |||
|
|
|||
| .editor-styles-wrapper { | |||
There was a problem hiding this comment.
This is really nice to get rid of, though.
|
Beautiful work, and high impact stuff. I left a few comments — the featured image placeholder might be a can of worms. One to open, definitely, but possibly in a separate location. In lieu of #35397 and #35242, I'm a fan of seeing these template blocks minimized:
The fact that the blocks output text such as "Post excerpt block: no post found." still makes it pretty obvious that you're not looking at content. But that's probably also something to keep in mind, if we were to reduce these phrases a little bit. Something to keep in mind for the effort. |
|
I reverted the Featured Image changes, we can tackle that one separately :) |
|
This is coming along. Post editor before: Post editor after:
Site editor before: Site editor after: The removal of the arbitrary dashed borders is a massive step forward, and I'm happy to see those land, so I'm happy to give this one a green check. I left a comment in #35517 (comment) about what we output, though, which may be worth thinking about in context of simplifying these. Looking at both of these as parts of the same, I can't help but feel that the added context of "Post Terms block: no post found." while verbose, at least gives some valuable information about the behavior of the block. Is there a way we can keep that extra context, while still simplifying? Perhaps "Post Terms (empty)"? I.e. |
I'm not sure it should be possible to insert things like the Post Categories block in the post editor (otherwise we need to account for weirdness like trying to output the categories assigned to a specific post, in a page 🤯). Concentrating on the template editor / site editor, we need to account for the following situations:
In each case you can swap 'categories' for any other term, for example 'tags'. Here's how we might approach each scenario:
Ideally the placeholders would be contextual, IE "Post Categories" instead of "Post Terms". What do you think? I'm not entirely sure that the "(empty)" suffix adds all that much value 🤔 |
|
I think I am possibly overthinking it, and I went ahead and approved the verbiage PR. This one might benefit from a rebase once it's merged. The thing is, we'll almost certainly want to circle back, and I find that it's always good to start as simple as is possible. |
|
Rebased and ready for another look :D
|
| @@ -25,7 +25,6 @@ | |||
|
|
|||
| .wp-block-post-author__name { | |||
| margin: 0; | |||
| font-weight: bold; | |||
There was a problem hiding this comment.
I realize this PR is not about the Post Author block. But seeing just this one font weight removed makes me wonder: why are there so many rules just for the editor view of the post author block? It seems like it would cause a lot of divergence. Any thoughts?
There was a problem hiding this comment.
I think it's because the Post Author block currently also includes an option to display the author avatar, bio, and a byline (I'm not entirely sure where the byline comes from 😬). These should all be separate blocks, but that is clearly another issue :D
There was a problem hiding this comment.
That's fine, whether one or multiple. My question is rather, why are these styles stored in editor.scss rather than style.scss. The latter is loaded in both the editor and the frontend, the former is only for editor specific affordances. Was just surprised by how many such there are.
There was a problem hiding this comment.
Ah I see. Yes that does seem odd. I'll see if I can tidy it up.
There was a problem hiding this comment.
Well, it doesn't have to be this PR, or you — I mostly noticed it because you tweaked the font weight, making me wonder: why just the font weight?
There was a problem hiding this comment.
Turns out we don't need the editor-specific styles for the Post Author block at all.
I think we can tidy these styles up even more when we split this in to separate blocks.
| @@ -24,7 +24,6 @@ | |||
| } | |||
|
|
|||
| &__name { | |||
| font-weight: bold; | |||
|
What's your confidence level on this one? It seems a small one to me, so I'll happily give a green check. |
|
I think it's looking good. Here's twentytwentytwo:
I would love to contextualise the post terms placeholders, but it's beyond my skills. It's probably something we can do in a follow-up, especially since they're also un-contextualised on trunk. |
Part of #35501.
Unifies the styling of site editor block placeholder states.
To test:
Note: the opacity applied to the Next/Previous post blocks seems to come from the
RichTextcomponent which I didn't want to touch here.