Closed douglas-johnson closed 3 months ago
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:
character between each block when using the inline layout.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?
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.
@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?
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!
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
@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!
@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
@douglas-johnson Thank you for this - I've not had a chance to do another review yet, but I'm not ignoring it!
Tagging @leogermani and @dhanson-wp for them to possibly do a review on behalf of their customer needs for this plugin.
Sounds good @GaryJones right now I have a few hours each week available for open-source projects and I'm ready to get this into production.
Thank you very much @jeremyfelt and @laurelfulford for reviewing this! I will do my best to address your feedback over the next week.
@laurelfulford I just pushed this commit https://github.com/Automattic/Co-Authors-Plus/pull/997/commits/bfd124495772c81419b87414cecad7b78bb60ca7
It is an attempt address these review items:
lineDashed
icon to represent the default layout which I feel matches well.Please let me know if you have any issues with video playback.
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.
https://github.com/Automattic/Co-Authors-Plus/assets/226381/19ebdc6a-c4b6-480d-a62b-717263028b6d
Showing the constrained layout settings working and alignments for the avatar block.
https://github.com/Automattic/Co-Authors-Plus/assets/226381/35062f30-e2a5-4c5b-8d8a-532864ec8bd3
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.
https://github.com/Automattic/Co-Authors-Plus/assets/226381/0a5d912f-8723-4fd4-91c1-47caceaa4d73
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.
https://github.com/Automattic/Co-Authors-Plus/assets/226381/cb119108-dd41-4fe4-b8d1-07a71557c3fa
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!
Hi @alecgeatches here are the few comments I think could still be addressed:
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? https://github.com/Automattic/Co-Authors-Plus/pull/997#discussion_r1412700433
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.
I suggested a fix here. I will take care of it ASAP. https://github.com/Automattic/Co-Authors-Plus/pull/997#discussion_r1412689745
Does this need to be addressed? As far as I know I don't have access to the relevant theme. https://github.com/Automattic/Co-Authors-Plus/pull/997#discussion_r1412696404
@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!! 🙌
@alecgeatches Based on what @laurelfulford said about the Avatar block dimension controls, I made an issue that I can pick up after this. https://github.com/Automattic/Co-Authors-Plus/issues/1022
Related Issues
940
932
Description
/coauthors-blocks/v1/coauthors/:post-id
to retrieve the co-authors of a post based on it's id./coauthors-blocks/v1/coauthor/:user-nicename
to retrieve a co-author based on their unique slug / user nicename.npm run build
classnames
package.Remaining Todos
How To Test
Block Editor
Full Site Editing
Extension
register_rest_field
to add information the thecoauthors-block
REST API responsescap/author
context as shown here in block.json and here in a render callbackQuestions
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 currentlyv16.19.0