-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix: Latest comment admin imports #8085
Conversation
08975fd
to
0e30658
Compare
function gutenberg_draft_or_post_title( $post = 0 ) { | ||
$title = get_the_title( $post ); | ||
if ( empty( $title ) ) { | ||
$title = __( '(no title)' ); |
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.
Text domain is missing
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.
Should be __( '(no title)', 'gutenberg' );
, right? 😄
@tofumatt: Is there any particular reason for using It seems that Core has had the behaviour of not displaying a title since forever, with no bug reports, which kind of makes sense: if a CPT doesn't allow a |
The idea here is the comments look very strange if you don’t have a post title. I suppose we could add logic to just show the comment. But then it would just be the commenter’s name, the date, and the comment (if enabled).
Having “No title” at least gives the comment a link to that post. "Recent posts” in core’s default theme do what I’m doing here, so it seems better to say “No title” than to show nothing (which might read more like a bug).
|
It can be done separately, but I found we could have slightly-improved support for WordPress functions via the (Still wouldn't have fixed the original issue on its own, though) |
In general, I think we should want to encourage avoiding dependency on |
* Copied from `wp-admin/includes/template.php`, but we can't include that | ||
* because: | ||
* | ||
* 1. it causes bugs |
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.
Could be more specific here.
* because: | ||
* | ||
* 1. it causes bugs | ||
* 2. it's in the admin; ideally we *shouldn't* |
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.
Incomplete sentence?
This is a fix to the latest comments block from #7941; apparently importing that admin file didn't work super-well and caused errors when re-generating fixtures, etc.
The function we're using is trivial to re-implement and I think the best solution for now. It also let me regenerate the fixtures.