Template Parts: Show existing template parts and a list of block patterns at creation flow. #38814
Conversation
|
Size Change: -1.4 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
| } ) { | ||
| const blockNameWithArea = area | ||
| ? `core/template-part/${ area }` | ||
| : 'core/template-part'; | ||
| const blockPatterns = useSelect( |
There was a problem hiding this comment.
The list of the patterns was created inside BlockPatternSetup. Do you think we still have the possibility of a reusable component for selecting patterns like BlockPatternSetup?
There was a problem hiding this comment.
I'm not sure, I left the current one because it's still used for other blocks (query...) but once we move to that block, we'd need to see.
For now I lean towards that not being needed but we'll see. The reason is that the template part modal is a bit different in the sense that it's composed of two areas (patterns and template parts) and the behavior of creating is a bit different between the two.
There was a problem hiding this comment.
For now I lean towards that not being needed but we'll see. The reason is that the template part modal is a bit different in the sense that it's composed of two areas (patterns and template parts) and the behavior of creating is a bit different between the two.
Maybe a solution would be for the template part to have two buttons choose a template part, and choose a pattern? But that makes the replace strange as we don't want two replace buttons.
There was a problem hiding this comment.
Yeah, I initially suggested that but @jameskoster thought it was simpler to just merge everything.
There was a problem hiding this comment.
Yup, I think the quicker we can show the user some options, the better.
There was a problem hiding this comment.
Maybe a solution would be for the template part to have two buttons choose a template part, and choose a pattern?
That is the current way, no? Also I agree that Template Part flow is different from other blocks which most commonly are uncontrolled blocks.
packages/block-library/src/template-part/edit/selection-modal/template-part-list.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/template-part/edit/selection-modal/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I guess it is going to be very common for a theme to create a pattern for each header and also a template part representing it, so we end up with duplicated lists:
Not a block but in the future, we may try to be smart and not show patterns if there is already.a template part with exactly that pattern.
|
If I add a template part block and then press choose the UI enters an infinite loop without showing any pattern: Video.MP4.1162x782.mp4I guess we have a bug when we have too many patterns to show? I guess we may consider showing the pattern explorer in the modal when the number of patterns is huge: Having search and categories for sure simplifies finding the pattern one wants in a big list. I guess we may also instead of showing a list for template parts and another list for patterns just show the pattern explorer with a tab for template parts when we are choosing something for a template part? |
|
When the number of template parts gets big I can not scroll and I'm never able to see the patterns: |
There was a problem hiding this comment.
I found some bugs in some hedge cases but nothing that could not be addressed in follow-up PRs. The code looks good and works well, so I'm approving the PR.
I wonder if it would make sense to reuse the patter explorer as the modal? Given that we would offer search, view by category, etc which may be useful when choosing a pattern for a very general block like a template part without an area set, where any pattern makes sense.
I'm not sure, that seems kind of superfluous to me. In most cases shouldn't a theme author decide on one or the other? |
Currently, 2022 has both patterns and template parts. So if it happens currently I guess it will continue to happen. Template parts are a must to allow the same header to be reused in multiple templates and allow changing all templates by just changing the template part. Patterns I guess is to allow to reuse of a header for example in another theme. |
In which case they should be submitted to the pattern directory rather than bundled with the theme, I think. Here's what I'm seeing so far:
I know this is WIP, but leaving a couple of notes / questions:
|
There was a problem hiding this comment.
Thanks for your work Riad! The combination of Template Parts and patterns do really simplify the logic/handling a lot!
This works well and follows the initial design about the creation/replacement flow.
Here are some extra thoughts...
Should we split this behavior for create and replace? For example if we replace with a pattern I feel like it should replace existing blocks and not prompt to create a new TP. This might fall under the block switcher/transforms category? Similarly to a patterns menu item it could show Template Parts and by clicking either one could trigger a UI with Modal like this one? 🤔
Another general thing that I've been thinking about the pattern explorer(s), as this modal is kind of a similar thing, is what if we could meddle with the block categories in patterns explorer? Right now we use the patterns categories but the component is being fed these from us. So what if adjust the categories in certain scenarios and use the same UI everywhere with categorization, search, etc.. In this case it would have two 'categories': template parts and patterns.. I haven't thought much the implementation but feel possible in my mind 😄
| closeLabel={ __( 'Cancel' ) } | ||
| onRequestClose={ () => | ||
| setIsTemplatePartSelectionOpen( false ) | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| isFullScreen |
There was a problem hiding this comment.
I tried this but the behavior is a bit weird, the width of the modal changes as the patterns load.
| } | ||
| .block-library-template-part__selection-content .block-editor-block-patterns-list { | ||
| display: grid; | ||
| grid-template-columns: 1fr 1fr 1fr; |
There was a problem hiding this comment.
I think it would look nicer if we adjusted the columns per viewport widths as we do in Pattern Explorer.
There was a problem hiding this comment.
Yeah, I also feel this logic should be a prop of the component instead of doing this in CSS.
packages/block-library/src/template-part/edit/selection-modal/index.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/template-part/edit/selection-modal/patterns-list.js
Outdated
Show resolved
Hide resolved
Not really, we don't have a concept of showing something before a block is inserted, that would require bigger changes. We can always share UI but there are different challenges there.
The theme can also define "patters" that are headers and they show on the second list. that's why I explicitly mentionned "template parts", the behavior is different when you pick a pattern (create a new template part), or pick an existing template part (reuse across templates) |
The double modal is needed even if we move the "start blank" outside the modal because when you pick a "pattern", you need to also create a new template part. |
I think that's a good point and one I would love more feedback on. cc @mtias @jameskoster |
Fair point, I'm still not sure we need to emphasise the "Template Part" label though. Especially as this is the Header block. "Theme Headers" or "Existing Headers" feels more intuitive to me.
Now I'm wondering how important the name really is in this flow :) I'm reminded of the friction that exists when we force folks to name their first navigation menu. Just an idea, but could we use the pattern name to auto-name the resultant template part? Starting blank could create "Header 1", etc.
Yes I think so. This is aligned with the conclusion we came to in #31747 |
Unfortunately we don't have a pluralized internationalized label for the areas, so these suggestions are going to be hard to implement. It's doable to add such pluralized label but it's not that simple. Can we create a follow-up issue for this one if needed.
I think it's important to name the template parts properly personally because these are the things that you'll refer to in multiple template to reuse the same header... Curious what other things here @jasmussen @mtias |
|
Is there any way to fix the jumping that occurs as patterns load? jumpy.mp4 |
|
@jameskoster personally I have a different experience, I don't see the same pattern being big and then small, but I do see patterns loading one after the other (it's intended for performance). There's no easy solution I can think of the moment 🤔 |
|
I think it may be related to the Site Logo and/or Navigation block. Another thought, since things like headers and footers tend to be wide and shallow, the 750px modal and 3 column grid do a rather poor job of previewing them. I appreciate there's a balance to be struck there, since we need to also consider narrow/tall patterns, but I think it might be worth trying either:
I understand if you prefer to do this separately. |
|
Took this for a spin, looks good! This is basic and works well. A few thoughts:
|
|
I've updated with the following changes:
|
I think that's fine for now. It also doubles as a cautionary "are you sure?" before embarking on a more advanced exercise. Two columns is working much better 👍 Agree with Joen that we need to do something about the jumping. A fixed height on the modal helps a bit, but it's still fairly awkward: fixed-heiht.mp4I don't know if it helps at all, but this seems to be caused by:
So perhaps it's something we need to fix separately on those blocks, or in the pattern preview itself... a loading state could be an option..? |
Can we open some issues about this, I think we should solve them separately of the current PR which just reuses the PatternList to render the previews. So if there's an issue here, there's probably an issue in the inserter and the patterns explore modal. I don't mind if these are considered blockers but at this point, I don't know how to solve them and I don't know how difficult they will be but they should be addresses specifically and not add more code to this PR. |
|
I'll open issues. Unsure if they should be blockers to this PR. I think less so if we apply a static height to the modal. What do you think @jasmussen ? |
|
Oh I'd think it great to move forward so long as we believe in the overall direction, and have some open issues for the todos we agree on 👍 👍 Edit: About the static height, I think we should add one! Or is there a component-specific reason not to add one? |
We can add one, no objections from me. What should it be? |
|
We can copy from the Preferences modal, which looks like it has a few complicated query based heights (would be nice to simplify those to just mobile and big), but it eventually settles on a 70% height, which seems good. |
cbravobernal
changed the title
cbravobernal
changed the title
part of #28737
I'm choosing to limit it to the template part block for now (setup states for other patterns based blocks like query... should be improved separately to reuse the same code here)
This PR updates the template part (and its semantic variations) to have a placeholder with a unique button that opens a modal. The modal shows a list of existing template parts to pick from and also a list of block patterns for the current semantic area.
Testing instructions