Skip to content
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

Block Library: Add a Post Author block. #19576

Merged
merged 6 commits into from Jan 14, 2020
Merged

Block Library: Add a Post Author block. #19576

merged 6 commits into from Jan 14, 2020

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Jan 11, 2020

Description

This PR adds a new Post Author block akin to the Post Title and Post Content blocks.

How has this been tested?

  • Inserted Post Author block in a post.
  • Confirmed post author rendered in the editor and front end.
  • Inserted Post Author block in a template.
  • Confirmed post author placeholder rendered in the editor and the relevant post author rendered in the front end.

Types of Changes

New Feature: There is a new Post Author block for template building.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

function PostAuthorDisplay() {
const [ author ] = useEntityProp( 'postType', 'post', 'author' );
return <address>{ author }</address>;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 13, 2020

Contributor

Potentially we could show a Post Author Select if the block is selected. We could start with a readonly block as well.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 13, 2020

Author Contributor

We still don't have an async Combobox component for this and I don't want that to block this structural PR.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 13, 2020

Contributor

We should use the same thing we already have for the Post Author in the document sidebar (imperfect) but I don't see this as mandatory for that PR anyway.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 13, 2020

Author Contributor

Agreed

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 13, 2020

Author Contributor

We should use the same thing we already have for the Post Author

Not on that though. Implementing that is so complex that it will be faster to implement the generic Combobox first and use that instead.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 14, 2020

Contributor

In my testing, this shows the id of the author, I believe we should show the username at least and it should be shown exactly like the fontend By %s

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 14, 2020

Author Contributor

Oops, the post object references author IDs. I've updated the code to fetch the actual author object and use its name instead.


export default function PostAuthorEdit() {
if ( ! useEntityId( 'postType', 'post' ) ) {
return 'Post Author Placeholder';

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 13, 2020

Contributor

i18n?

This comment has been minimized.

Copy link
@epiqueras
if ( ! $post ) {
return '';
}
return '<address>' . __( 'By ' ) . get_the_author() . '</address>';

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 13, 2020

Contributor

Should this use sprintf?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 13, 2020

Author Contributor

What would be the benefit?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 13, 2020

Contributor

In some languages, there might be no need for "by " or the author should come first. Ideally concatenation of translated strings and non translated ones should be avoided as much as possible because the sentence format changes from language to another.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 13, 2020

Author Contributor

Oh, right. I totally forgot about the translation here.

Thanks for catching it.

Fixed!

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 13, 2020

Added "design feedback" tag for designers to start thinking about the potential of these blocks (style variations, styling...)

@epiqueras epiqueras force-pushed the add/post-author-block branch from fc9a078 to 5ec16de Jan 13, 2020
@epiqueras epiqueras requested review from nerrad and ntwb as code owners Jan 13, 2020
@epiqueras epiqueras mentioned this pull request Jan 13, 2020
3 of 9 tasks complete
( select ) => select( 'core' ).getEntityRecord( 'root', 'user', authorId ),
[ authorId ]
);
return author ? <address>{ sprintf( __( 'By %s' ), author.name ) }</address> : null;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 14, 2020

Contributor

In slow connection, there's a jumpiness happening when loading the author. This should be solved with design somehow.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 14, 2020

Author Contributor

We need loading block placeholders for these "async blocks."

@epiqueras epiqueras merged commit 6118c92 into master Jan 14, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras added this to Done in Phase 2 via automation Jan 14, 2020
@epiqueras epiqueras deleted the add/post-author-block branch Jan 14, 2020
return '';
}
// translators: %s: The author.
return sprintf( __( '<address>By %s</address>' ), get_the_author() );

This comment has been minimized.

Copy link
@ockham

ockham Jan 15, 2020

Contributor

This is causing a lint error on WP.com when attempting to update the plugin there:

!! Please put the opening and closing HTML tags outside of the translatable string:
wp-content/plugins/gutenberg-core/7.3.0-alpha/build/block-library/blocks/post-author.php:19 __( "<address>By %s</address>" )

which seems fair. @chriskmnds is working on a fix.

If core contributors feel that a lint check this makes sense, is there any chance we can add one to CI?

/cc @akirk

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

Why is this an issue? It would just make the code harder to read with the concatenations to what end?

cc @aduth @youknowriad

This comment has been minimized.

Copy link
@akirk

akirk Jan 15, 2020

Contributor

The suggestion is to have translators only translate By %s. For one, it lets the translator focus on the text. Secondly, there is possible translation reuse because By %s has already been translated. Finally, if the HTML tags are added through code, it means you can change them later to add CSS classes, ids, whatnot, without having to change the translations.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 15, 2020

Contributor

It makes easier for translators because the html tag in that case will just be repeated. It doesn't make sense to allow translators to change the HTML tag.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

That makes sense.

Should I open a PR or are you already on it?

This comment has been minimized.

Copy link
@chriskmnds

chriskmnds Jan 15, 2020

Contributor

I have a fix, but putting together the PR properly, testing, etc. will take a while (even for something simple as this). :D

This comment has been minimized.

Copy link
@chriskmnds

chriskmnds Jan 15, 2020

Contributor

#19675

will test tomorrow, but feel free to review pls :-)

This comment has been minimized.

Copy link
@aduth

aduth Jan 29, 2020

Member

For posterity's sake, there is a follow-up task related to this conversation at: #19911

@ellatrix ellatrix modified the milestones: Future, Gutenberg 7.3 Jan 20, 2020
@karmatosed karmatosed added this to Done in Full site editing Feb 4, 2020
@kjellr kjellr added the New Block label Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.