-
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
Reveal block boundaries on hover in the Site Editor #27271
Conversation
Size Change: +38 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
@@ -208,7 +208,7 @@ | |||
|
|||
// Active entity spotlight. | |||
&.has-active-entity:not(.is-focus-mode) { | |||
opacity: 0.5; | |||
//opacity: 0.5; |
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.
Probably need to remove this when this PR goes out of draft state.
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.
Probably 😅
Thank you for this PR. As will be evident from the GIF below, my site editor is currently a little messy, so it's not totally accurate. But hopefully a few aspects can serve as legitimate feedback: First off, the rendering issues with the 1.5px border are present here. I've a bit of tasks on my plate today, but I'm happy to try and dive in tomorrow and see if I can help fix it. Secondly, and as noted in #27239, I think we should avoid different colors for template part boundaries. Right now we have black and blue colors (in the default color scheme), and those are sufficiently challenged on black and blue backgrounds, that I think we need a smarter system for rendering colors on arbitrary backgrounds before we introduce additional ones. In this one, there's both some weirdness with the hover style (there's a dead zone). But also the fact that it looks like the border of the placeholder box grows thicker. This is probably a coincidence, because the hover is 1px outset from the border of the placeholder. I wonder if we should have a spot colored 1px hover style, instead of black? This would make blue the color for all things selected/focused:
What do you think? |
Updated to focus on hover interactions only for now (removed template part colors). I think the selected / focused / hovered outlines are behaving as expected, but I am not 100% familiar with the interaction there so more eyes would be helpful. One question I have there is: which treatment takes priority? IE if I hover a selected item, which outline style should prevail, 1px (hovered) or 1.5px outline (selected)? |
There are no rules to follow here, so it's what we choose as best. Would it be useful for a focused block to have a hover effect at all? Note, I figured out why the focus outline is weirdly cropped like so: If you add The benefit is that this focus style will also be visible in windows 10 high contrast mode. |
Updated so that the selected style persists over the hover style. Since the hover style intends to outline what will be selected on click, it's probably not adding much value when the block is already selected. |
Current status: This is looking like a good small step that I think is worth trying. And most of the issues appear to have been fixed. It looks to me like some of the blue borders are used incorrectly in the site editor — that's the case also in the master branch so not the fault of this PR — but they linger even if focus moves elsewhere. The thick blue border is focus only, so we should look at refactoring it to actually be attached to |
Thanks for exploring! We might want to leave the outline visible on select as well until is-typing kicks in. |
In exploring this it feels good as I interact. The hitches come as mentioned where certain behaviours outside the scope of this clash against the hover. This seems outside the scope here, but I mention as it could do with easing to ensure the experience flows as it can distract. For example: Similarly here the issue with mis-aligned borders and numerous states is a little more obvious: |
Seems related: #27075 |
This is a solid first step. I think the subtle differences between 1px, 1.5px, 3px, blue, and black borders is going to be misunderstood by most people. I find the current 1px, solid, blue line distracting but am willing to live with it as we improve as it does solve a number of real problems. I'd suggest moving away from box-shadow hacks and rounded corners, and sticking with the simple CSS |
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.
I like it!
For some reason, the template part border is the same color for me. If that's intentional, then I'm curious why we need extra classes to handle borders for the template part block specifically? It seems like it could just be generic. I also agree with shaun re: not using box-shadows, but I'm ok moving forward with this to see how folks like it!
@@ -208,7 +208,6 @@ | |||
|
|||
// Active entity spotlight. | |||
&.has-active-entity:not(.is-focus-mode) { | |||
opacity: 0.5; |
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.
If we are removing the template part spotlight mode thingy, we can probably remove this class entry altogether
It's intentional for now, we might re-explore later. The only template part specific class I can see is to handle the offset on the :after, but it doesn't look like we really need that, so I'll remove for now. |
Updated based on your review @noahtallen, thanks for that. I also moved the outline styles to what I believe is a more appropriate file. |
Im curious why we are limiting this change to the site editor? Is the idea to try it out here and potentially move these over to the post editors if it proves out well? It seems like a lot of the interactions this aims to improve are not site editor specific. While it is noted above that block nesting is featured more prominently in this context by default, it can still be just as (or more) prominent in the post editors based on the users needs and designs that are being implemented there. |
It'd be cool to have it as a setting. Maybe it would be off by default there, and on by default in the site editor. (maybe that could be a followup?) |
By default, this effect could be jarring on the post editor alone (we tried it a while ago) overemphasizing blocks too much. I think eventually, if this iteration works out for the better, it should be part of the "select" tool. The site editor could make the select tool the default, then. |
TestingRequirementsI tested template parts and a variety of different blocks (image, paragraph, etc.)
Browsers
I did notice a bit of wonky behavior with placeholders. @jasmussen pointed out issues with dead zones earlier, and I'm seeing them as well. Overall, I think the code changes look reasonable, and the editing experience, to me, definitely feels more intuitive than before. Thanks for working on this @jameskoster 🙌 |
This PR roughly implements parts of a prototype I worked on for #27239, to see how it feels more holistically.
Block boundaries are exposed on hover making it easier to understand what will be selected upon click. This is important in the Site Editor particularly, because block nesting features prominently in that context.
A different color is applied to template parts, to indicate that they are different to regular blocks.
This feels like a net positive change to me, although I would say that the
is-hovered
class seems to be somewhat brittle. Moving the cursor around quickly can trip it up. It doesn't occur all that often, but if there's any way to improve performance there it would be worth exploring.To test: