Query: Ensure create new post link is above block styles #49566
Closed
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
aaronrobertshaw
added
[Type] Bug
|
Size Change: +3.77 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e51184c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4685422528
|
talldan
reviewed
Apr 6, 2023
packages/block-editor/src/components/create-new-post-link/index.js
Outdated
Show resolved
Hide resolved
ntsekouras
reviewed
Apr 6, 2023
| @@ -326,6 +327,7 @@ const BlockInspectorSingleBlock = ( { clientId, blockName } ) => { | |||
| className={ blockInformation.isSynced && 'is-synced' } | |||
| /> | |||
| <BlockVariationTransforms blockClientId={ clientId } /> | |||
| { blockName === 'core/query' && <CreateNewPostLink /> } | |||
There was a problem hiding this comment.
This is very specific to core/query block and is probably better not to live in block-editor package.
An alternative, that is in need in general, is a new SlotFill inside BlockCard component. We could also consider just relocating the specific link inside Query Loop settings or something.
There was a problem hiding this comment.
Thanks for questioning this @ntsekouras 👍
Initially, when the block inspector tabs were introduced we explored the addition of a SlotFill to the BlockCard. That approach was discarded due to concerns that 3rd parties could and would abuse the slot to promote their controls to higher prominence than warranted. We can't really lock down the SlotFill to internal use only as anyone that determines the name will be able to render fills to it.
There is also a precedent with the BlockCard component having core/navigation block-specific code which led to the current approach proposed in this PR.
As for relocating the link elsewhere, I'm not sure there is much appetite for that. In the original issue that added this link, there were struggles in finding a proper home for the link and this was the only place that was agreed upon.
Would you have any objections to proceeding to land this fix and have the potential relocation of the link continue in a follow-up?
There was a problem hiding this comment.
Quite a lot have changed in UI since the introduction of this link, so maybe before proceeding with this approach we could have some extra input from @jameskoster or @jasmussen maybe, if there is a place we could relocate the link and not avoid adding the specific block functionality in block-editor package.
There was a problem hiding this comment.
I'll refer to Jay if he has input, he's deeper into the query stuff. But from a simple before and after, the after looks like an improvement. That's mostly ignoring the question of whether we need this link in the first place, or whether there's a better interface for adding new posts.
There was a problem hiding this comment.
If we want to keep the link then I'd agree with Joen, this is a visual improvement. But the UX leaves a lot to be desired:
- It ejects you from the current edit state which is particularly jarring when you get sent from the site editor to the post editor.
- "For this feed" is often untrue.
- It's a bit tricky to interpret when editing templates like Archive, Category, Tag.
The whole thing ideally needs a fresh design exploration. But as low-hanging-fruit follow-ups I'd suggest updating the label to read "Add new post" which would address point 2 above. Bonus points if we can make it post type aware. E.g. if I configured a custom query to display products, then the label would read "Add new product".
There was a problem hiding this comment.
I've tweaked the wording of the new post link as suggested to "Add new post".
Making it display the translated name for the post type might be more involved as I don't think we can simply pull out the post type data from the @wordpress/core-data store within the block editor package. I could look into that as a follow-up to this though.
There was a problem hiding this comment.
Excellent thoughts Jay. That reminds me that we have an unfinished task for the split inspector followup, and those notes may be relevant in revisiting this:
- Explore a semantic slot, an ellipsis menu next to e.g. the query loop title.
- Ellipisis menu can have a third party hook for things like a premium block “badge”, “this is a new block” unread dot, links to meta actions (add new post, etc).
- Explore a “Styles” panel revamp that includes more context for the Styles tab, help text, variations, copy styles, etc.
There was a problem hiding this comment.
Making it display the translated name for the post type might be more involved as I don't think we can simply pull out the post type data from the @wordpress/core-data store within the block editor package
I think you can do select( 'core' ).getPostType( postType ) to get the post type object, which includes the labels:
{
"name": "Posts",
"singular_name": "Post",
"add_new": "Add New",
"add_new_item": "Add New Post",
"edit_item": "Edit Post",
"new_item": "New Post",
"view_item": "View Post",
"view_items": "View Posts",
"search_items": "Search Posts",
"not_found": "No posts found.",
"not_found_in_trash": "No posts found in Trash.",
"parent_item_colon": null,
"all_items": "All Posts",
"archives": "Post Archives",
"attributes": "Post Attributes",
"insert_into_item": "Insert into post",
"uploaded_to_this_item": "Uploaded to this post",
"featured_image": "Featured image",
"set_featured_image": "Set featured image",
"remove_featured_image": "Remove featured image",
"use_featured_image": "Use as featured image",
"filter_items_list": "Filter posts list",
"filter_by_date": "Filter by date",
"items_list_navigation": "Posts list navigation",
"items_list": "Posts list",
"item_published": "Post published.",
"item_published_privately": "Post published privately.",
"item_reverted_to_draft": "Post reverted to draft.",
"item_scheduled": "Post scheduled.",
"item_updated": "Post updated.",
"item_link": "Post Link",
"item_link_description": "A link to a post.",
"menu_name": "Posts",
"name_admin_bar": "Post"
}
There was a problem hiding this comment.
Is that not the @wordpress/core-data store? I did search the codebase for getPostType and only saw uses of the core-data store which is restricted in the block-editor package.
jameskoster
approved these changes
Apr 12, 2023
|
I appreciate we would rather not have block-specific code in the block-editor package here despite the precedent with the navigation block code in the Given that, I'm leaning towards getting this merged to fix the initial issue and buy time for further explorations on removal or relocation of the link. Any objections @ntsekouras or @talldan? |
talldan
reviewed
Apr 13, 2023
packages/block-editor/src/components/create-new-post-link/index.js
Outdated
Show resolved
Hide resolved
talldan
approved these changes
Apr 13, 2023
There was a problem hiding this comment.
LGTM.
I don't think it's a long term fix but solves the immediate issue. Hopefully there's a better UI on the horizon, or agreed upon extra extensibility in the BlockCard.
Another option that springs to mind is allowing slots to be properly private in a similar way to other private APIs. Perhaps using a symbol for the slot name or a private reference. That could help solve a lot of problems.
ntsekouras
approved these changes
Apr 13, 2023
| @@ -326,6 +327,9 @@ const BlockInspectorSingleBlock = ( { clientId, blockName } ) => { | |||
| className={ blockInformation.isSynced && 'is-synced' } | |||
| /> | |||
| <BlockVariationTransforms blockClientId={ clientId } /> | |||
| { blockName === 'core/query' && ( | |||
There was a problem hiding this comment.
Can we just add a comment, that this should leave with the first chance we get(either relocation of CreateNewPostLink inside Query or any other way)?
There was a problem hiding this comment.
Good call. I can definitely add a comment before merging this.
Dan and I were discussing our options around creating a private SlotFill. My plan is to explore that a little tomorrow. If it looks like something we can get in soon, then we'll opt for that instead of this PR 🤞
|
Thank you everyone for the reviews, ideas, and nudges towards a better solution. I am going to close this PR for the time being in favour of #49819 which will avoid the block-specific code in the block-editor package. |
1 task
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.
Fixes: #49327
What?
Moves the query block's "Create new post" link so that it will render above any block styles registered by plugins.
Why?
The block inspector's layout appears broken when the "Create new post" link doesn't immediately follow the block card and any block variation transforms.
How?
CreateNewPostLinkcomponent from the query block into the block editor packageCreateNewPostLinkto retrieve the attributes from the stores currently selected blockCreateNewPostLinkcomponent in the block inspector for single block selections when the current block is a Query blockAlternate Approach
One possibility I looked into was to resurrect a previously tried SlotFill approach but this time lock it as a private API. This turned out to be a much heavier change, touching more areas, and still wouldn't 100% prevent the concerns that blocked #45437.
Testing Instructions
Screenshots or screencast