Improve guidance to editing template when focused on editing a page #51366
Conversation
noisysocks
added
[Type] Enhancement
|
Size Change: +514 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
I implemented the flashing hackily for now. Will improve once @SaxonF is keen on the design. |
|
Definitely an improvement and both worth implementing. One tiny detail, and this might be annoying to implement, is to slowly fade away the border highlight after click giving the user enough time to notice. We might also want to use the hover highlight colour vs the selected (white). |
|
Nice one, took it for a spin and here's a gif of me doubleclicking to edit the template
I'm noticing that as I click slowly enough to not invoke the double click, I'm seeing the borders flashing a bit, I don't see that in trunk:
Is it intentional those borders flash up? One note, on mobile double-tap is commonly zoom. That does seem mostly handled here, in that double-tap doesn't actually invoke the edit mode. I think it's fine that this is the case, and that to edit a template on mobile you have to go into the inspector, so I'm just validating that it's okay for double tap not to do that here. But I'm also seeing those flashing borders on mobile, which feels more awkward there:
|
Yes, the idea is to highlight what fields can be edited when you click on something that cannot be edited. I could look at only flashing the borders after we're sure that the current click is not a part of a double click?
I think it makes sense to not have the double click gesture on mobile since it is usually the gesture for zoom, like you say. I'm not sure about whether to flash the focus ring on mobile. Thoughts @SaxonF? On the one hand mobile has the same problem we're trying to address which is that it's not always obvious which content is editable. On the other hand the "focus flash" pattern is something I've never see on mobile. It's usually a desktop idiom accompanied by an alert sound.
👍 sounds good. Worth noting is that I'm using an existing animation that we have in Gutenberg so we should be consistent. I'll try to make the existing one fade in/out too. |
|
I added a 300ms timeout so that we don't flash the outline when double clicking. I looked into making the outline fade in and out but don't think we should touch it because |
noisysocks
changed the title
|
Flaky tests detected in f7ff0dc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5306907039
|
|
The double-click seems worth a try. I'm not sure the outline effect is working too well, especially with the delay. It feels a bit clunky / unexpected to me. If I am trying to select a template block, it's not really obvious why these other blocks are being highlighted. outlines.mp4Another issue worth fixing is that the "Edit your template to edit this block" Snackbar only seems to appear once. So if you miss it the first time (easily done), you're left entirely in the dark. |
|
OK let's keep it simple for now. I updated this to:
|
|
Thanks for fixing that. How do you feel about double-click taking you directly to edit template? At the moment, double-click and single-click are essentially producing the same result, the only difference being that one invokes a modal and the other a snackbar. The only other thing I'm missing is a reference point to "this block" mentioned in the snackbar/modal. Sometimes clicking seemingly empty space produces that message which feels a bit strange. Example:
Probably something to try in a different PR, but I wonder if showing the block outline (purple) on mousedown would help. |
We can create a follow up issue that has a "don't ask me again" setting that just takes people to editing template on double click. Main goal right now is to educate as much as possible on why something isn't selectable. Single click is less intrusive. Double click a little more in your face.
I like this idea. Let's get this one merged so its ready for 6.3 and do a follow up to show outline on mousedown. |
|
I explored the This PR just needs a code review. I'll request reviews from some folks who might be able to help with that. |
| { hasPageContentFocus && ( | ||
| <> | ||
| <DisableNonPageContentBlocks /> | ||
| <EditTemplateDialog contentRef={ contentRef } /> |
There was a problem hiding this comment.
A nice follow up, if possible, would be to prevent <EditTemplateNotification /> from rendering when the dialog is open.
There was a problem hiding this comment.
By putting the notification behind a timeout? Not sure how this could be done 😀
There was a problem hiding this comment.
No idea either, but the double notification isn't ideal.
Not sure how to do it either without probably a refactor.
Just noting it.
There was a problem hiding this comment.
Yeah maybe we can dismiss the notification automatically when modal appears. I'd have to smoosh the components together though. I guess that's not so bad.
packages/edit-site/src/components/page-content-focus-manager/edit-template-notification.js
Show resolved
Hide resolved
| } } | ||
| onCancel={ () => setIsOpen( false ) } | ||
| > | ||
| { __( 'Edit your template to edit this block' ) } |
There was a problem hiding this comment.
Not a biggie for this PR, but this text can be misleading since you can click anywhere outside a content block to trigger the notice + modal, even on the latters' padding or margin.
Maybe "Edit your template to edit this area" ?
There was a problem hiding this comment.
Tiny nit: for text within the dialog, should we end the sentence in a full-stop? Also, not a blocker (and wording can be tweaked in follow-ups), but since we have a modal, I was wondering if it's worth including more explanatory text in this one. E.g. something along the lines of, "You are currently editing ${ pageTitle }, do you wish to switch to template editing?"... though now that I write that out, I'm not sure how much more helpful that is. Writing this sort of micro copy is hard! 😅
There was a problem hiding this comment.
Will add the periods! 👍
Not 100% sure about changing the copy. Wdyt @SaxonF?
There was a problem hiding this comment.
this sort of copy is indeed difficult. I'd be fine shipping as is and follow up with a copy pass.
packages/edit-site/src/components/page-content-focus-manager/edit-template-notification.js
Show resolved
Hide resolved
| } } | ||
| onCancel={ () => setIsOpen( false ) } | ||
| > | ||
| { __( 'Edit your template to edit this block' ) } |
There was a problem hiding this comment.
Tiny nit: for text within the dialog, should we end the sentence in a full-stop? Also, not a blocker (and wording can be tweaked in follow-ups), but since we have a modal, I was wondering if it's worth including more explanatory text in this one. E.g. something along the lines of, "You are currently editing ${ pageTitle }, do you wish to switch to template editing?"... though now that I write that out, I'm not sure how much more helpful that is. Writing this sort of micro copy is hard! 😅
There was a problem hiding this comment.
This is testing well for me, too!
I noticed a couple of tiny issues that also exist on trunk, so not a blocker for this PR:
Double-clicking on a template block above the Post Featured Image block appears to cause focus to be redirected to the Post Featured Image block in addition to the modal displaying. I wonder if it's possible to prevent the double click behaviour from resulting in the focus shift? Probably not a big issue:
2023-06-19.14.49.03.mp4
When the page title is selected for editing, it looks like the block toolbar is still rendering, but empty, resulting in a tiny black dot rendering above the post title block:
Oh, I see this PR's just been merged now 😄... don't worry about my comments here, nothing that would have been a blocker to merging!
packages/edit-site/src/components/page-content-focus-manager/edit-template-notification.js
Show resolved
Hide resolved
| alreadySeen, | ||
| prevHasPageContentFocus, |
There was a problem hiding this comment.
Nit: There's no need to add mutable values like refs as dependencies. When you provide them, alreadySeen.current ESLint shows a warning.
noisysocks
added
the
[Feature] Page Content Focus
…ordPress#51366) * Improve guidance to editing template when focused on editing a page * Don't flash content blocks when double clicking * Use flashBlock() * Remove content block flashing for now * Show edit template notification if not already showing one * Oops, this doesn't belong here * Restore packages/block-editor from trunk * Optimise if() * Add periods to notifications * Dismiss notification when opening dialog
What?
Follows #50857.
Adds a few more UI affordances intended to guide the user from page focus mode to template focus mode.
Why?
Users are getting confused when they click on a block that's a part of their template and it doesn't do anything. See #51304.
How?
Testing Instructions
Screenshots or screencast
Kapture.2023-06-09.at.17.09.53.mp4