Closed douglas-johnson closed 1 year ago
Hello Doug! Thank you for your contributions in this issue. There are a lot of great ideas here. You've identified some valuable missing features (like block support) for the plugin that we'd be happy to have. We've looked over your PR changes and would be happy to support you in getting these merged into the main plugin. We appreciate the care and time you've put into your changes and design questions.
Features I'd like to add
- Incorporate separators to display coauthors in an inline list. I imagine this would work similar to the Post Terms block
- Add support for appearance tools, especially fonts.
- More author data:
- Author archive URL, with an option to make the display name a link.
- Avatars, preferably as an image block similar to the post feature image block. Also with an option to make it a link.
- Author bio
- Subscribe to coauthor data so if the authors are changed in the sidebar, that is reflected in the block content.
A solid yes for all of these!
What is the ideal way to fetch the author data in the block editor?
After reviewing the code, I have some ideas on how I'd approach the limitations of the current sidebar-focused REST API. Please let me know what you think!
- I used apiFetch and
/coauthors/v1/authors/:post-id
for this version.
- The data in that endpoint seemed especially suited for the existing plugin sidebar.
- It includes coauthor emails, and is rightfully restricted to people with the capability to assign authors. This can cause permissions errors for people with other roles.
I experimented with a regular author
assigned to a post, and saw the permissions errors you mentioned when trying to view the new Co-Authors block. It seems strange that an assigned user is not able to see other assigned users due to the permissions model and data.
I think adding a new author data REST endpoint for block data specifically would work here. This should be limited to users assigned to a specific post. Co-authors on a post should be able to query for other author information for block rendering, but only for the posts that they're assigned. The API can provide author data necessary to render blocks without trying to shoehorn data into the more restrictive sidebar API. Ideally we'd use the same code in REST responses that we use for dynamic block rendering to pull relevant author information.
- The avatars are URLs. It would be nice to have the attachment id ( when one is used ) for the purpose of using the Image Block.
Good idea! A separate REST endpoint could include this and any other information you need to render blocks.
- I tried using the existing store for its getAuthors function, but found it had a side-effect where it also setAuthors. When I used getAuthors within a query loop, the post I was editing suddenly had 10 authors.
- There isn't an endpoint yet where I can fetch an author by id, so when providing context to the display name block, I literally passed the display name. In the example of the Post Title block, the provided context can just be the postId. From there I think a store can be used to avoid repeat API calls.
The getAuthors()
side-effect is strange but seems fixable from a quick look. Because we only call getAuthors()
in a couple of places, we can change the calling code to trigger setAuthors()
after existing getAuthors()
calls and remove the nested call.
However, if we use a separate REST endpoint + store as pitched above this fix may not be necessary. With a new store we could potentially subscribe to the existing sidebar updates and use this to auto-update co-author block data without needing to alter the sidebar behavior.
- In the past when I needed to display co-author data for posts fetched from the API, I added a
profile
value to the author taxonomy terms. https://github.com/thoughtis/cata-co-authors-plus/blob/0.6.1/includes/api/class-api.php
- This is non-standard, but maybe we could standardize it.
The profile
trick is useful, but since it's non-standard I'd prefer to stuff extra data in a separate locked-down endpoint.
Hopefully those can help address your questions. These answers are my personal opinion, so if you have different ideas or pushback, please let us know.
I encountered a couple of other questions testing the PR:
package.json
that refer to the parent's installed node_modules
. Would it be possible to consolidate block building to a single npm run build
command under the root package.json
?cap/coauthor-display-name
block doesn't appear to show anything when inserted directly into a post. It only renders inside of a query block (like your example), or when used as an inner block of cap/coauthors
. This was unexpected behaviour to me. How should that block look when inserted outside of a query or wrapping block context?Thank you!
Hi @alecgeatches thank you for all of this feedback. I will follow up as soon as I can with an implementation plan for your review. I'd like to address the Other questions
section though.
Blocks have separate package.json that refer to the parent's installed node_modules. Would it be possible to consolidate block building to a single npm run build command under the root package.json?
Do you have any guidance or documentation on how to do that? I have tried multiple times and never succeeded.
The cap/coauthor-display-name block doesn't appear to show anything when inserted directly into a post. It only renders inside of a query block (like your example), or when used as an inner block of cap/coauthors. This was unexpected behaviour to me. How should that block look when inserted outside of a query or wrapping block context?
It shouldn't be inserted outside of a query or wrapping block context. It should be updated to be restricted to those contexts, with the possible addition of author archive templates.
Blocks have separate package.json that refer to the parent's installed node_modules. Would it be possible to consolidate block building to a single npm run build command under the root package.json?
Do you have any guidance or documentation on how to do that? I have tried multiple times and never succeeded.
@wordpress/scripts
at some point added support for multiple blocks by scanning for block.json
. It expects a src/
directory by default, but I was able to build on your fork by changing the root build
script with --webpack-src-dir
:
"scripts": {
"build": "wp-scripts build --webpack-src-dir=./blocks",
// ...
}
This dumps the built blocks into a different build/
directory, so it'll need some tweaking to work correctly with new paths. Hopefully that'll do the trick.
The
cap/coauthor-display-name
block doesn't appear to show anything when inserted directly into a post. It only renders inside of a query block (like your example), or when used as an inner block ofcap/coauthors
. This was unexpected behavior to me. How should that block look when inserted outside of a query or wrapping block context?It shouldn't be inserted outside of a query or wrapping block context. It should be updated to be restricted to those contexts, with the possible addition of author archive templates.
Makes sense to me!
Thanks @alecgeatches
@wordpress/scripts at some point added support for multiple blocks by scanning for block.json. It expects a src/ directory by default, but I was able to build on your fork by changing the root build script with
--webpack-src-dir
. This dumps the built blocks into a different build/ directory, so it'll need some tweaking to work correctly with new paths. Hopefully that'll do the trick.
That change in the build folder was part of what I considered unsuccessful last time I tried this. The distributable block.json and scripts are no longer colocated with the PHP for calling register_block_type
and handling the render_callback
. I suppose that can be handled.
Do these two issues also occur for you? I am reasonably sure I'm running the latest @wordpress/scripts
. When I ran the build command as you demonstrated, it also created an incomplete /src
folder within the /build
folder for each block:
and the block.json in the build folder for each block includes nonexistent CSS files:
That change in the build folder was part of what I considered unsuccessful last time I tried this. The distributable block.json and scripts are no longer colocated with the PHP for calling
register_block_type
and handling therender_callback
. I supposed that can be handled.
That's annoying for the organization, I agree. I imagine most people just deal with it.
and the block.json in the build folder for each block includes nonexistent CSS files:
That's definitely weird, and I'm seeing the same behavior. I think it may have to do with the source layout, as wp-scripts build
is expecting a flat block folder with block.json
and associated files rather than a tree with build/
and src/
.
What do you think about stringing together some build commands instead? This setup seems to work with the existing build structure:
"scripts": {
"build": "npm run build:plugin && npm run build:block-coauthors && npm run build:block-coauthor-display-name",
"build:plugin": "wp-scripts build",
"build:block-coauthors": "wp-scripts build --webpack-src-dir=blocks/coauthors/src/ --output-path=blocks/coauthors/build",
"build:block-coauthor-display-name": "wp-scripts build --webpack-src-dir=blocks/coauthor-display-name/src/ --output-path=blocks/coauthor-display-name/build",
I'm more interested in the convenience of a single npm run build
command than doing everything with wp-scripts
defaults, so this is a fine solution for me as well.
@alecgeatches That makes sense to me. I may investigate more to see if there are ways to simplify, but we'll get to a single build command somehow.
Can we discuss workflow?
I imagined I would:
Would you be able to provide feedback on those issues and help decide which should apply to the v1 milestone? I expect I'll also ask for reviews on the pull requests.
Does this work or is the an alternative strategy that's more appropriate?
@alecgeatches That makes sense to me. I may investigate more to see if there are ways to simplify, but we'll get to a single build command somehow.
@douglas-johnson Sounds great! I'm sure there's smarter way to shorten or combine those steps.
I imagined I would:
- Enable issues on my fork.
- Create issues and a v1 milestone.
- Make pull requests to the main branch of the fork.
- Make a pull request to this respoitory when the v1 milestone is satisfied.
Would you be able to provide feedback on those issues and help decide which should apply to the v1 milestone? I expect I'll also ask for reviews on the pull requests.
The downside with this approach is that it would move the conversation and code discussion from this plugin's repository to your fork. It would be great if we can find a workflow that will keep more of that in this repo.
Would this work for you?
Would this work for you?
Sure, I can make that work. I'll close this issue and start a new tracking issue.
I am interested in displaying coauthor data in block editor and full site editing contexts.
I started a branch on a fork where I put together two blocks for this purpose. Here is a link to compare it to the current master version: https://github.com/Automattic/Co-Authors-Plus/compare/master...thoughtis:Co-Authors-Plus:block-exploration
I am interested in feedback from both users and maintainers. Depending on the feedback I am happy to either submit a pull request, or keep this as a separate project. Thanks for taking a look.
What it does
coauthors
block which works like the Post Template block.coauthor-display-name
block.Example
Screenshot of the editor where I'm outputting the display name of the authors in both the current post and a query loop. The stored and rendered versions of this post content are included at the end of the issue.
Features I'd like to add
Questions about potentially incorporating this into Co-Authors Plus
What is the ideal way to fetch the author data in the block editor?
/coauthors/v1/authors/:post-id
for this version.profile
value to the author taxonomy terms. https://github.com/thoughtis/cata-co-authors-plus/blob/0.6.1/includes/api/class-api.phpExamples continued from above
The stored content:
The rendered content: