Post content block: create content select pattern placeholder #57572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
changed the title
|
Size Change: +292 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ramonjd
added
[Type] Experimental
3 tasks
|
Thanks for tackling this, and CC @SaxonF if he hasn't already seen it. Since this is a draft with a todo mentioning it, I'm assuming the following is already known, but we should avoid the dark-bordered placeholder pattern for this particular solution. |
ramonjd
commented
Jan 8, 2024
ramonjd
force-pushed
the
try/post-content-select-pattern-placeholder
branch
from
ca61284 to
04a9f97
Compare
noisysocks
reviewed
Jan 9, 2024
noisysocks
reviewed
Jan 9, 2024
noisysocks
reviewed
Jan 9, 2024
noisysocks
reviewed
Jan 9, 2024
noisysocks
reviewed
Jan 9, 2024
|
Flaky tests detected in 98e7908. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7456792556
|
ramonjd
changed the title
ramonjd
requested review from
draganescu,
talldan,
tellthemachines,
adamziel,
kevin940726,
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
ramonjd
commented
Jan 9, 2024
| const hasInnerBlocks = !! entityRecord?.content?.raw || blocks?.length; | ||
|
|
||
| const initialInnerBlocks = [ [ 'core/paragraph' ] ]; | ||
| const hasInnerBlocks = !! blocks?.length; |
There was a problem hiding this comment.
I've changed this line so that the placeholder only cares whether the post content block has any child blocks, and will show even if there's saved post content.
This allows us to show the placeholder in the post editor in template preview mode as well.
Do folks see any downsides here?
There was a problem hiding this comment.
The failing E2E tests are real and I think due to this change.
I think maybe reverting this change and then do a quick follow up to this PR to avoid a large change set.
youknowriad
reviewed
Jan 9, 2024
| @@ -78,6 +78,7 @@ _Parameters_ | |||
| - _value_ `boolean|Object`: Whether the inserter should be opened (true) or closed (false). To specify an insertion point, use an object. | |||
| - _value.rootClientId_ `string`: The root client ID to insert at. | |||
| - _value.insertionIndex_ `number`: The index to insert at. | |||
| - _value.initialCategory_ `string`: The tab category to display first when the block editor inserter is opened. A category corresponds to one of the tab categories defined in packages/block-editor/src/components/inserter/tabs.js. | |||
There was a problem hiding this comment.
Can you explain the need for this new API for both the actions and the inserter component? What's this about?
There was a problem hiding this comment.
Thanks for taking a look.
The value initialCategory determines which tab displays when the inserter is opened.
At the moment, the initial tab is blocks.
For example, when setInserterIsOpened is called with a value of: initialCategory: 'patterns', the patterns tab will open first.
Another option is 'media'.
If none of those tabs exist, it will default to blocks.
The requirement is to be able to control this behaviour from the block editor.
The value is accessed via the private selector.
This seemed to me to be the most obvious place to add the property at the time, especially seeing as its related to opening the inserter, but I'm very aware doesn't mean it's an appropriate place/method 😄
If you have suggestions I'm very keen to make this better!
There was a problem hiding this comment.
I don't know if I have great suggestions here, adding it as part of the "insertion point" state could work but
- It should probably not say "category" but "tab" right?
- Can we make it "private" everywhere?
- Can we avoid experimental/unstable prefixes?
There was a problem hiding this comment.
Thanks for the tips!
It should probably not say "category" but "tab" right?
👍🏻
Can we make it "private" everywhere?
Do you mean a new private action?
As with rootClientId and insertionIndex (getInsertionPoint), the only way to access the property value is through the private selector in @wordpress/editor.
The other object properties rootClientId and insertionIndex are already out there in the wild as part of the "public" API.
The sidebar inserter state I assume should remain in the @wordpress/editor package (?)
I'll have to take a closer look at the public setIsInserterOpened action, which lives in the @wordpress/editor package. setIsInserterOpened appears to be exported in useBlockEditorSettings from @wordpress/editor presumably so it can be accessed by blocks and elsewhere, e.g.,
// From a block
select( blockEditorStore ).getSettings().__experimentalSetIsInserterOpened( {
rootClientId: clientId,
insertionIndex: 0,
} )Can we avoid experimental/unstable prefixes?
👍🏻
This PR introduces __experimentalInitialCategory/Tab.
I'm also happy to create a new follow up PR that removes __experimental* for __experimentalFilterValue and __experimentalInsertionIndex.
Cheers!
Adding hook to set placeholder
Only use initialTabId if a corresponding tab exists
…ed E2E test changes and more manual testing in post and site editors.
update e2e tests remove `_experimental` Flag for initialTab prop
ramonjd
force-pushed
the
try/post-content-select-pattern-placeholder
branch
from
7560cba to
f7f0a92
Compare
andrewserong
approved these changes
Jan 22, 2024
There was a problem hiding this comment.
This is testing nicely for me!
✅ Shows placeholder while editing a template:
✅ In template preview mode, the choose a pattern and blank buttons work nicely:
It also looks like the private selector structure here is consistent with the feedback on this PR so far 👍
This LGTM, @SaxonF did you have any other changes you wanted to make? This looks good to land to me, and styling or copy changes could be made in follow-ups, potentially (e.g. making sure text works for custom post types as mentioned in: #56898 (comment)).
Comment on lines
+56
to
+59
| const label = | ||
| 'page' === postType | ||
| ? __( 'This page’s content is empty' ) | ||
| : __( 'This post’s content is empty' ); |
There was a problem hiding this comment.
Should we have a fallback string for when the post type is neither a post nor a page? I.e. something like:
let label = __( 'This content is empty' );
if ( 'page' === postType ) {
label = __( 'This page’s content is empty' );
} else if ( 'post' === postType ) {
label = __( 'This post’s content is empty' );
}
noisysocks
added
[Type] Enhancement
|
@SaxonF and I are going to experiment with some lighter touch approaches to this. The problem with using |
noisysocks
added a commit
that referenced
this pull request
Jan 25, 2024
Co-authored-by: ramon <[email protected]>
2 tasks
This was referenced Jan 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Using most of @SaxonF's ideas from #56898 🙇🏻
Why?
How?
Testing Instructions
Screenshots or screencast
2024-01-09.14.20.05.mp4