Template Part: Improve insertion flow. #23295
Conversation
epiqueras
added
[Type] Enhancement
epiqueras
requested review from
ajitbohra,
Soean,
spacedmonkey,
talldan and
TimothyBJacobs
as code owners
|
Size Change: -470 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is great so far! I really like it.
A couple questions about the dropdown:
- Should the Dropdown / preview panel be wider? It seems very thin, but it does behave well and it doesn't necessarily look bad.
- There are some circumstances where the dropdown has 2 scroll bars. Maybe the height of the preview container should be contained within the dropdown? That is, so the outer scrollbar wouldn't be triggered, the 'search' input would always be at the top of the dropdown, and the 'scroll' would only happen through the preview window itself?
I really like the feel of how the new template part is inserted. Both having the renaming input and getting rid of the default paragraph seem like a big step up.
This name panel also helps with #22064, making it more apparent that you are editing a template part. (I suppose I need to get used to calling them 'Sections' 😆 )
Do you think this flow will change frequently enough that it's worth disabling E2E tests for it, for now, to ease iteration?
After this gets updated for whatever immediate concerns design has, id think it would be worthwhile to update the tests again. If you want to skip them for now, Id be happy to help update the tests as a follow up (if it makes sense and things seem semi-stable for the time being).
We should probably try to keep the safety net in place if there is going to be time between iterations. Id also assume a fair amount of the main concepts of this design will remain in place for a while, in which case 🤞 we are doing 'minor' updates to the tests going forward after this as opposed to re-writing them.
|
This is already a great improvement. I think we'll need to stick to using the term "Template Part" for this PR. Let me know when you're ready for a review.
@epiqueras I don't think functionality will change drastically, but perhaps some of the stuff around reusable block UI or the inserter could change. I trust your judgement here. |
Yeah, I made it wider.
Yes, fixed.
Yeah, that's good, it'll also look better when we move it to the toolbar.
Why?
It's ready 😄 |
I'm worried it could hold up the PR, but maybe not? It is still an experimental feature so we can definitely experiment. Maybe add "template part" as search term so it shows up when searching the inserter |
|
I think the sooner we switch, the better, and it makes it look a lot better. |
🤣 - I was sitting there typing "/template" for the block search multiple times out of habit. |
Let's do it. |
There was a problem hiding this comment.
Im noticing a tab accessibility regression with this implementation. I am only able to tab through the first couple sections in the preview list before having the dropdown closed by the 5th 'tab':
I'm not quite sure what the deal is, it doesn't correspond with the end of the theme group in the preview or anything else. I can't get past the 4th Section to select any of the further ones. It does seem to be something with this dropdown as the current preview list tabs through to the end. 🤔
@Addison-Stavlo This is a good point I hadn't thought of. It does have me thinking the idea to shift to a text input in the block toolbar would be a better approach, but we need more design feedback there first. |
|
Updated tests 🚀 |
@Addison-Stavlo For me, it always closes at the last item. But, the same thing happens for patterns, so this is not a regression of this PR. |
For me it goes all the way through the entire list with the patterns in the block. With the dropdown it cuts out early. 🤔 |
|
This is coming along nicely. I just took this for a spin and so far there are still a few things to do before this is ready to merge:
The reusable block title bar looks like this: Right now, this is what I'm seeing for the Section title bar: It looks like it's just an input currently. I would like to see it match the Reusable block UI closely including the Edit/Save button.
Here is the latest inserter from the master branch:
And here is what I'm seeing for the Section inserter:
|
How would that work with the "Review Changes" modal? I thought we were moving away from a specific/local save button.
Yes, these are separate components. I went with what was already being used for template parts. IMO, the current design is not ideal, but it also shouldn't look like the patterns inserter. We have to make it look good for the different heights of template parts and group them by theme and name somehow. |
I suppose we could do without the edit/save button, but I think they are valuable - particularly for editing the name of the Section. It should still look identical even without the save button which means there's some wrapping container to go around the input.
I understand that they are separate components and that won't look exactly like the current inserter. I do expect it to be pretty close to the mockups. Here was the latest mockup from the proposal:
|
I would assume if you 'save' on the toolbar, it would no longer appear in the 'review changes'. Similarly, if you edited and did not hit save, it would then be present in the 'review changes' save flow.
So it tabs through all items in the preview list for you? How many items do you have present in that? I created a handful of different styled template parts so I had about 8 in mine. When I first tested it would get through about the first 4 before breakout, after adding another template part it now closes after the very first item in the list. There definitely seems to be something funky with the tab flow in the dropdown. |
I'll update it.
I was planning to work on that in a follow-up PR since this is already improving on what's already in
That feels a bit redundant. @mtias Have you thought about this before?
Yeah, like eight too. |
|
@youknowriad Any idea what's going on here? |
|
@epiqueras I'm not sure what the question is, some context could be helpful. It seems to be about tabbing. The dropdown close itself if it loses focus, so maybe that's what happening here (focus transferred programmatically elsewhere). For example: when a block gets selected it receives the focus automatically, maybe it's related. |
|
@youknowriad The popover which shows available template parts with almost identical code to what the patterns inserter uses, closes after tabbing through the first non-empty template part. |
|
Are you cloning the blocks to preview or not, maybe you're using the same "clientIds" as blocks rendering (which can mean selected too) |
|
No, the blocks are unique because they are parsed from the template part HTML. |
epiqueras
force-pushed
the
update/template-part-insert-flow
branch
from
8e20287 to
e816f68
Compare
|
@Addison-Stavlo Fixed! Let's ship this!
|
There was a problem hiding this comment.
This works considerably better. Do you know what this issue was? I tried testing / undoing the changes from that last commit but it seems to work better regardless of that (was there a fix elsewhere in the rebase?).
I did notice another issue in a preview containing links (footer from 2019-blocks). On tabbing, the links steal focus and subsequently close the dropdown again before I can get to the next pattern in the list:
Also - An unrelated note / question regarding how we should handle the namebar for inserting theme supplied template parts. Currently after inserting the name is blank:
The popover was fixed in
This is a limitation with the block preview that is also present in
It should display correctly now. |
Im a bit conflicted on this point. While it is a limitation with previews, there doesn't seem to be any other implementation of them where this limitation breaks a11y? So while the limitation with previews itself shouldn't block the PR, the fact that implementing this design forces us to introduce that limitation in an area that creates further concerns with a11y is what makes me feel uneasy. Does that make sense? If this is a limitation we can fix, then it seems all will be good. But if we cannot fix this limitation in previews, then this dropdown preview design itself is further limited and cannot be fully accessible. Thoughts? |
|
Patterns with links also break, right? It shouldn't be difficult to enhance the preview to block tabbing into it. |
Im not finding any with links to test, but presumable they would if they existed (in the dropdown / new quick-inserter). Since we are still under the |
There was a problem hiding this comment.
Great job here, thank you for all these updates!
The code looks great and everything tests well from my end, minus the noted underlying limitation with some previews stealing focus (#23295 (comment)). Since FSE is still 'experimental', it seems ok to move forward with this despite the limitation (the majority of cases seem to work quite well). But we should look to be aware of this, try to fix the underlying problem, and if all else fails and this is causing issues we may need to figure out some alternative to the dropdown down the road.
|
I think it's worth noting that the name "Section" may be confusing for some users who are expecting an HTML The naming may also be confusing for people expecting something more like the Group block. In a lot of page builders, "section" refers to a container element, not a global template part. |
Description
This PR improves the template part flow by:
We should follow this up with an exploration of moving the renaming input to the toolbar.
@MichaelArestad @shaunandrews Do you think this flow will change frequently enough that it's worth disabling E2E tests for it, for now, to ease iteration?
How to test this?
Play around with inserting "Section" blocks in the FSE experiment.
Screenshots
Checklist: