-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add: Author nicename template creation ability #42165
Conversation
Size Change: +975 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
packages/edit-site/src/components/add-new-template/new-template.js
Outdated
Show resolved
Hide resolved
Thanks @jorgefilipecosta. It's a little awkward to refer to an author as an "item" 🙈 Something like this might work better, and simplify things: We could probably do the same with the other templates as well, and it would still make sense: What do y'all think? |
2b0b236
to
e90d25c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ntsekouras, and @jameskoster, thank you for the reviews! All the feedback was applied.
9f1eea3
to
2e92398
Compare
Thanks for your work here Jorge! Let's wait a bit here while considering a possible re-architecture of the add templates implementation. |
{ sprintf( | ||
// translators: %s: Lowercase Label to signify all items. E.g.: 'all Posts'. 'all Pages', 'all Categories', 'all Authors', etc. | ||
__( | ||
'Select whether to create a single template for %s or a specific one.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're missing 'all':
Select whether to create a single template for all authors or a specific one.
Or is that included in all_items
?
2e92398
to
7568a5f
Compare
This PR was updated taking into account the recent architecture done by @ntsekouras. I also removed the copy changes so they can be discussed in a separate PR specific to them, given that there were some concerns related to i18n. |
Thanks for updating 'item' -> 'author' on the buttons @jorgefilipecosta, much better! :) Do you think it would be possible to make the same change in the paragraph, and the button sub-labels? We may even be able to do some reduction: What do you think? |
Hi @jameskoster, it sounds like a good plan. I will follow this feedback related to labels as it may affect all templates to a separate PR, and in this one, we can keep just the author addition so the PR can be merged faster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work Jorge!
de6e643
to
3584d7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing, clicking on the Author menu item never opens the modal, it always starts a new generic author
template. I also get the following network error:
http://BLABLA/wp-json/wp/v2/users?context=view&per_page=1&_fields=id&who=author&_locale=user
{
"code": "rest_invalid_param",
"message": "Parâmetros inválidos: who",
"data": {
"status": 400,
"params": {
"who": "who não é authors."
},
"details": {
"who": {
"code": "rest_not_in_enum",
"message": "who não é authors.",
"data": null
}
}
}
}
3584d7d
to
a37cb7d
Compare
I missed something (an "s") during my commit. These issues were fixed. |
Hi @mcsf the issue was fixed and your suggestion applied, the PR is ready for another pass. |
8aa9d4c
to
8560afc
Compare
Hi @ntsekouras you feedback was applied 👍 |
8560afc
to
8775943
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some small tweaks Jorge! This is almost ready.
We still need a needsUniqueIdentifier
function with different logic to handle users with the same display name
.
Screen.Recording.2022-07-29.at.11.25.21.AM.mov
let authorMenuItem = defaultTemplateTypes?.find( | ||
( { slug } ) => slug === 'author' | ||
); | ||
if ( ! authorMenuItem ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add the item here even if the author
template doesn't exist in the default templates, like we do for the other entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied the feedback to be consistent with other entities, but I think as follow-up, we should not add a template even if it does not exist on default templates.
We are breaking backward compatibility. Currently a plugin can disable a template by removing it using the filter default_template_types in:
/**
* Returns a filtered list of default template types, containing their
* localized titles and descriptions.
*
* @since 5.9.0
*
* @return array The default template types.
*/
function get_default_block_template_types() {
$default_template_types = array(
'index' => array(
'title' => _x( 'Index', 'Template name' ),
'description' => __( 'Displays posts.' ),
),
'home' => array(
'title' => _x( 'Home', 'Template name' ),
'description' => __( 'Displays posts on the homepage, or on the Posts page if a static homepage is set.' ),
),
'front-page' => array(
'title' => _x( 'Front Page', 'Template name' ),
'description' => __( 'Displays the homepage.' ),
),
'singular' => array(
'title' => _x( 'Singular', 'Template name' ),
'description' => __( 'Displays a single post or page.' ),
),
'single' => array(
'title' => _x( 'Single Post', 'Template name' ),
'description' => __( 'Displays a single post.' ),
),
'page' => array(
'title' => _x( 'Page', 'Template name' ),
'description' => __( 'Displays a single page.' ),
),
'archive' => array(
'title' => _x( 'Archive', 'Template name' ),
'description' => __( 'Displays post categories, tags, and other archives.' ),
),
'author' => array(
'title' => _x( 'Author', 'Template name' ),
'description' => __( 'Displays latest posts written by a single author.' ),
),
'category' => array(
'title' => _x( 'Category', 'Template name' ),
'description' => __( 'Displays latest posts in single post category.' ),
),
'taxonomy' => array(
'title' => _x( 'Taxonomy', 'Template name' ),
'description' => __( 'Displays latest posts from a single post taxonomy.' ),
),
'date' => array(
'title' => _x( 'Date', 'Template name' ),
'description' => __( 'Displays posts from a specific date.' ),
),
'tag' => array(
'title' => _x( 'Tag', 'Template name' ),
'description' => __( 'Displays latest posts with a single post tag.' ),
),
'attachment' => array(
'title' => __( 'Media' ),
'description' => __( 'Displays individual media items or attachments.' ),
),
'search' => array(
'title' => _x( 'Search', 'Template name' ),
'description' => __( 'Displays search results.' ),
),
'privacy-policy' => array(
'title' => __( 'Privacy Policy' ),
'description' => __( 'Displays the privacy policy page.' ),
),
'404' => array(
'title' => _x( '404', 'Template name' ),
'description' => __( 'Displays when no content is found.' ),
),
);
/**
* Filters the list of template types.
*
* @since 5.9.0
*
* @param array $default_template_types An array of template types, formatted as [ slug => [ title, description ] ].
*/
return apply_filters( 'default_template_types', $default_template_types );
}
If we ignore the default code that currently removes a template will stop working because we add the template anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a tricky one we have to think better. When the filter was introduced though we didn't have this kind of functionality for adding more templates. We might need more feedback about what would be expected.
Another thing is that while this filter exists, users cannot actually add extra items, as they are filtered in js. The most obvious example would be the home
template that already exists in this list but is filtered out in js..
Hi @ntsekouras, I don't think we need to handle users with the same display name, we are showing the link for the user which contains the slug: People can use the link the distingish between different users, adding the slug to the name, I think in this case would just add confusion. |
That's true for the suggestions list, but if we create templates for two authors like that, the template title/description in the main list is identical - I show this in my video above. |
Co-authored-by: Miguel Fonseca <[email protected]>
14d4b47
to
6e933ed
Compare
Hi @ntsekouras, @jameskoster, I added an update for the authors, but I guess we should also do it for posts and taxonomies? That made me think maybe we should not store in the template name and description the title of the post or a category name at a given moment. The title of the post can be changed (or the name of the author or the name of the category), and it seems strange that the template name does not reflects that. Maybe we should not store template names and descriptions and they are computed dynamically based on the current names (by the rest endpoint)? |
Hm... that's true. I guess the reason I mentioned that for authors would be mostly stemming from some personal experience where authors shared the same name. I'd suggest to keep this for authors only for now, as a more common use case for folks to have the same name, and wait to see if any feedback pops up about the posts etc.. as it's more unusual to have the same titles for posts of the same post type, etc.. Also the dynamic calculation could be something we could explore, but I think we can wait and see first too and avoid complicating some logic for now.. What do you think? |
Yes good observation, we will need a holistic solution here.
This has been on my mind recently as well. "What happens to the template name if I change the title of the source entity". Like you said, things can get confusing quite quickly when you start changing post/post type/taxonomy/term titles.
That would seem ideal. As long as it also remains possible for a theme to supply a |
I pushed some small tweaks. The user facing one was to add the conflicting slug only in title as we do for the other entities.
This is even more complicated if users start changing slugs and not the names. IMO we shouldn't rush into adding all the complexity yet to this before getting more feedback, but 🤷 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work here Jorge! I think it's good to land and iterate if needed.
Agreed. I'll open an issue to discuss further. |
Thank you for the reviews and enhancements @ntsekouras, @jameskoster 👍 |
This PR adds an option to create an author-nicename template on the site editor UI.
It uses the same UI as Posts and Taxonomies.
Part of: #37407
The unrequited horizontal scroll is an issue already on posts and taxonomies and should be addressed separately.