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

Blocks for Co-Authors #997

Merged

Conversation

douglas-johnson
Copy link
Contributor

@douglas-johnson douglas-johnson commented Sep 7, 2023

Related Issues

Description

  • Added 2 REST API endpoints
    • GET /coauthors-blocks/v1/coauthors/:post-id to retrieve the co-authors of a post based on it's id.
    • GET /coauthors-blocks/v1/coauthor/:user-nicename to retrieve a co-author based on their unique slug / user nicename.
  • Added 5 blocks for use in the block editor and full site editing
    • Co-Authors
    • Co-Author Avatar
    • Co-Author Description
    • Co-Author Featured Image
    • Co-Author Name
  • Added a Redux store that provides customizable placeholder content for use in full site editing when the author cannot be known.
  • Added a custom webpack configuration file so it is possible to build the existing sidebar plugin and the new blocks with a single npm run build
  • Added a dependency on the classnames package.

Remaining Todos

  • Improve iconography, categorization and description of blocks in their block.json files.

How To Test

Block Editor

  • Create a Query Loop and add the Co-Authors Block to the post template.
    • Customize the prefix, suffix and separators in the inline layout.
    • Customize the vertical block spacing in the block layout.
  • Update the Co-Author Name block so their name is a link to their author archive.
    • Customize the link color or other style settings.

Full Site Editing

  • Add the Co-Authors block to the single template.
    • Use the inline layout for bylines.
    • Use the block layout for a larger "about the author(s)" section.
  • Add the individual Co-Author blocks like Name, Description and Featured Image to the author template without wrapping them in the Co-Authors Block.

Extension

  • Use register_rest_field to add information the the coauthors-block REST API responses
  • Use the methods described in the updated portion of the readme to add corresponding placeholder data.
  • Create a custom block that displays the new author information and utilizes the placeholder data in full site editing.

Questions

Can both of the REST API endpoints related to blocks be public?

In our initial discussion @alecgeatches requested that the API endpoint that allows someone to view the co-authors of a post be restricted to users who can edit that post and the co-authors themselves. I was on board, and coded it that way, but have a different perspective now that I've worked on it.

In my opinion, the REST responses do not contain any sensitive information. The author schema can be reviewed here: https://github.com/thoughtis/Co-Authors-Plus/blob/issue%23940/coauthor-blocks/php/api/endpoints/class-coauthors-blocks-controller.php#L198

Leaving the API endpoints public allows them to be used on the server-side, which allows blocks to use the same schema in both the editor and server-side render callbacks. It is very convenient.

As an example, I have opened up the permissions for the endpoint that retrieves a single author based on their user-nicename and I am using it on the server-side here: https://github.com/thoughtis/Co-Authors-Plus/blob/issue%23940/coauthor-blocks/php/blocks/class-blocks.php#L144

I would like to do the same for the endpoint that retreives an array of co-authors based on a post id.

What are the WordPress version support requirements for blocks?

Since blocks are in active development and full site editing is relatively new to WordPress core, some block-related features just don't work in older versions. For example the automated tests include WordPress 5.7 which doesn't support registering a block type through block.json

Is it reasonable to only activate the blocks for WordPress 6.3 and up?

Is there an established Node version that should be used?

I built everything using Node lts/gallium which is currently v16.19.0

@douglas-johnson
Copy link
Contributor Author

One more question, this one's about whitespace differences between HTML produced by JSX in the editor and the content saved to the database.

When producing the "inline" layout for the Co-Authors Block in JSX, there is no whitespace. So for example if you want your author's avatar to be spaced apart from their name, you can use margin.

Those same blocks include line-breaks between each block when stored in the database, so that same avatar and author name now have both whitespace and margin in the rendered content.

I found two possible ways to handle this:

  1. In JSX, add a   character between each block when using the inline layout.
  2. In PHP, remove line-breaks between the inner blocks of the Co-Authors Block.

For now I chose option 2, and it's being handled here:
https://github.com/thoughtis/Co-Authors-Plus/blob/issue%23940/coauthor-blocks/php/blocks/block-coauthors/class-block-coauthors.php#L145-L148

So ultimately the question is... is handling it in the PHP the least hacky way to handle this, or is the better way I'm not thinking of?

@GaryJones
Copy link
Contributor

is handling it in the PHP the least hacky way to handle this

That works for me.

@ douglas-johnson Can you please tidy up the commit history for this PR into atomic meaningful commits? Understanding the purpose of a commit (and more than just "an arrow function I forgot to do in the previous commit") will help with the review. You can force-push your feature branch to GitHub when done.

I'll go ahead and review it anyway, as the changes I'll ask for can then be incorporated into the right commits as you rewrite it.

@douglas-johnson
Copy link
Contributor Author

douglas-johnson commented Sep 10, 2023

@GaryJones I’m happy to amend that specific commit, which had to do with PHP 7.1 compatibility.

I haven’t been asked to rewrite commit history before, so I am not sure how far to go. Should I just combine adjacent related commits? Or reset the head and commit each changed file with new messages?

Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglas-johnson Firstly, THANK YOU for all the work you've put into this. I'm thrilled that the plugin users will finally benefit from blocks with CAP!

I'm keen to get your feedback (agreement or push back) on the comments I've left.

Some are questions borne of my lack of knowledge around React/block editor, so I'm happy to take them as learning opportunities if you're willing.

Other comments are a bit more opinionated but I'm happy to discuss any further. For many of these, I've only left a single comment on one instance with the expectation that all instances of that issue would be changed across the files as needed.

build/blocks/block-coauthor-avatar/block.json Outdated Show resolved Hide resolved
build/blocks/block-coauthor-avatar/block.json Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
php/blocks/class-blocks.php Outdated Show resolved Hide resolved
src/blocks/block-coauthors/edit.js Outdated Show resolved Hide resolved
src/blocks/block-coauthors/edit.js Outdated Show resolved Hide resolved
src/blocks/block-coauthors/edit.js Outdated Show resolved Hide resolved
@GaryJones
Copy link
Contributor

I haven’t been asked to rewrite commit history before, so I am not sure how far to go. Should I just combine adjacent related commits? Or reset the head and commit each changed file with new messages?

If the commits are granular enough, then you can change the commit order so that related commits are together, and then squash them as necessary. For instance, there might be a commit for adding endpoint 1, another for endpoint 2, 5 commits for adding the individual blocks, maybe more before or after for adding supporting functions, etc.

What it's not, is making each commit = one file, as that's rarely going to be the whole mini-feature. I'll give this example of a PR where it's possible to review each commit as an atomic change, fully described from the commit message. From my own PRs on this repo, I'll share this example and another example if that makes it clearer.

If anything more than squashing adjacent commits seems like too much hard work (I use SourceTree to manage my commit and history rewriting), then don't worry - I value the time you've already spent on this, and would take the end result over a clearer git history!

@douglas-johnson
Copy link
Contributor Author

I made this gist to demonstrate how to extend the REST API responses and placeholder data in order to add a custom block that uses the author context. I wasn't sure how an example like this would fit into documentation but I thought it would be useful.

https://gist.github.com/douglas-johnson/3bea50c83d2d75e0024d332474f468dc

@alecgeatches
Copy link
Contributor

@douglas-johnson Just wanted to let you know I'm actively reviewing your PR. There's a lot of ground to cover in this PR, so it will take some time to test and review code, but you should expect to hear back from me shortly. Thanks!

Copy link
Contributor

@alecgeatches alecgeatches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglas-johnson Incredible work! Your block additions are carefully implemented and held up well to all of my testing. I love that you've designed for extensibility, and I'd like to include your Co-Authors Plus extension block example in the plugin's wiki after this is merged if that's okay with you.

I've requested a handful of changes, but they're mostly surface-level. Thank you so much for these changes to modernize Co-Authors-Plus, I'm excited to get them in soon!

Also thank you @GaryJones for your prior comprehensive review! That made things a lot easier for me.

Copy link
Contributor

@alecgeatches alecgeatches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglas-johnson Thank you for the updates, everything is looking good to me now. Thank you so much for the work you've put into this PR.

@GaryJones I'm good to merge this whenever you are.

@douglas-johnson
Copy link
Contributor Author

@alecgeatches Excellent!

I haven't addressed permissions for the /coauthors API endpoint yet, so it shouldn't be merged just yet.

When that's in I'll re-request a review from @GaryJones

* Adds `get_authors_with_api_schema` function that returns the co-authors from the WP_REST_Server so the schema and data match the editor.
* Uses that new function in the Co-Authors Block when getting the authors for a post.
Co-authors who are WordPress users do not have a type property. So checking it to make sure they are a guest author fails.
- Switch from figure element to div to eliminate need for removing margin. Same as default WP avatar.
- Eliminated redundant display:inline, handled by parent.
- Removed alignfull and alignwide styles which are not supported.
- Updated render function to correctly support vertical-align styles.
- Updated all blocks to use apiVersion 3.
- Updated use of `Co-Authors` for consistency.
- Updated readme for consistency with block UI.
@GaryJones GaryJones removed their request for review December 15, 2023 09:31
@GaryJones GaryJones dismissed their stale review December 15, 2023 09:31

Dismissing my review as Laurel is taking over the ownership of reviewing this PR.

- Added block layout controls to the Co-Authors block.
- Updated Co-Authors block CSS so the default / flow layout displays co-authors in an inline style suitable for bylines.
- Added context that is passed from Co-Authors block to its descendants so controls only relevant to the inline style can be hidden when using other layouts.
- Removed support for align from the block.json file of the image and avatar blocks, instead adding it manually in the block’s edit.js so it can be omitted when using the defaut layout. When using other layouts the align controls will be removed where not applicable by `useAvailableAlignments`.
@douglas-johnson
Copy link
Contributor Author

@laurelfulford I just pushed this commit bfd1244

It is an attempt address these review items:

  • Image alignments were available in contexts where they don't work.
  • Layout icons did not intuitively match the layouts.
  • Block inspector controls specific to one layout where always available.

Examples

Please let me know if you have any issues with video playback.

Bylines

The layouts that are available for the group block and post template block are all available to layout the Co-Authors block. Since its not yet possible to add a custom layout to the defined set of default, constrained, grid, flex I am using default for the inline layout.

co-authors-plus-bylines-converted.mp4

Card One

Showing the constrained layout settings working and alignments for the avatar block.

co-authors-plus-card-one-converted.mp4

Card Two

This part hasn't changed much, I'm mostly showing that you can still use Row and Stack layouts within the Co-Authors block to layout each individual author.

co-authors-plus-card-two-converted.mp4

Archive

Alignments for avatar and image blocks are available only where determined by useAvailableAlignments so you can see here I need to put the image in a group to use aligncenter.

co-authors-plus-archive-converted.mp4

While using the site editor `outlineMode` is enabled, which can obscure the cursor when hovering over the RichText field for the prefix attribute of the Co-Authors Block. The missing cursor makes it unclear you can type in the prefix field.
@alecgeatches
Copy link
Contributor

You've been doing fantastic work @douglas-johnson getting just about every little issue ironed out in this PR. Your original PR was already a huge improvement to this plugin, and I think at this point we've been prolonging a merge too long.

@laurelfulford @jeremyfelt @GaryJones do you have any objection to merging as-is? I'd like to get this functionality part of the main plugin. And of course @douglas-johnson if you'd like to wait on a merge for other reasons, let me know. Thank you!

@douglas-johnson
Copy link
Contributor Author

Hi @alecgeatches here are the few comments I think could still be addressed:

Feature parity with core avatar block

This is worth addressing in the near future, but is an improvement not a bug. Should I work on it next week or make an issue out of it?
#997 (comment)

In WP 6.4 the core avatar block switched from a pre-determined set of image sizes to a range control. Since Co-Authors Plus makes use of media library images which only have a pre-determined set of sizes, I would need to figure out how to snap the requested image size to one that is actually available.

Using smaller default size for Co-Author Featured Image block

I suggested a fix here. I will take care of it ASAP.
#997 (comment)

Reported issue with newspack block theme

Does this need to be addressed? As far as I know I don't have access to the relevant theme.
#997 (comment)

Full was considered too big, especially in template editor contexts.
@laurelfulford
Copy link

@alecgeatches No objections from me!

@douglas-johnson I'm sorry it's taken me so long to get back to this! I tried retesting the two issues you mentioned in your latest comment and added comment to them. In summary, I think the featured image approach looks good, and I can't recreate that weird issue with the template parts anymore -- that paired with the fact you couldn't reproduce it with a default theme makes me think it was user error, or a fluke.

Thanks again for taking on this awesome work!! 🙌

@douglas-johnson
Copy link
Contributor Author

@alecgeatches Based on what @laurelfulford said about the Avatar block dimension controls, I made an issue that I can pick up after this. #1022

@alecgeatches alecgeatches changed the base branch from develop to release/3.6.0 April 12, 2024 15:46
@alecgeatches alecgeatches merged commit 8650ea3 into Automattic:release/3.6.0 Apr 12, 2024
3 checks passed
@alecgeatches alecgeatches mentioned this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants