-
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
Block Library: enhance the author block #19894
Conversation
@mapk this PR implements most (all?) of the functionality needed by the new author block, but It doesn't look best yet. One thing though: I added an avatar size selector to the inspector. Using block styles to play with the avatar size, like in the mock-up, would mean that we use the avatar as a background instead of an image, and I think it's not OK. The avatar's URL depends on both the author and the size, so we can't change that based on style (or can we?) 😁 |
b1d46cf
to
8edda56
Compare
8edda56
to
4508f91
Compare
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.
I'm having trouble testing this PR correctly. In the instructions, where do I "Create a single template for full site editing"? I tried to do so in Appearance > Templates, but can't find out how to apply the resulting template to a post.
If I skip that step, I am able to add a Post Author block to a regular post, but although it displays in the editor, on the front end "No matching template found" is rendered as the post title.
In the editor view, it would be good to have some padding on the left of the block content:
packages/block-library/src/post-author/block-colors-selector.js
Outdated
Show resolved
Hide resolved
Thanks for wrangling this PR, @draganescu! After fiddling with the block a bit, here's some feedback.
I wonder if the byline should mimic the "citation" in the Quote block. So this way it's there ready for the user to type something, but when clicking away from the block, it disappears if nothing is present.
Reducing the spacing to 10px helps add a consistent grid throughout. See below: |
f74b10f
to
7565b18
Compare
The text alignment button in the toolbar doesn't seem to do anything, and I'm not sure it should even be there at all. Similar, while the color button does work, I wonder if its needed or required. I've been trying to find a way to surface some of the options from the sidebar into the toolbar. I've ended up with this "Display" menu: Its a little generic, but I think it helps "unhide" all these options which are currently in the sidebar. Also, having the byline on it's own line seems a little much. Often, I'd expect this to literally just contain "Written by:" or similar language, and it could look strange to force two lines when a single line (byline + authorName) seems to work better. |
I'm glad you're exploring ways to bring over these bits to the block. Looking at this, I wonder if we should just call it "Settings" instead of "Display"? Does this open up the ability to add all settings to the block toolbar? The Accessibility Team, I think, would love that to happen. But before it does, I do struggle with clear rules for when a "Display" dropdown should be used over the Sidebar. Do you have an idea on how we might communicate the difference to developers?
Your byline works beautifully. |
This is a complex block for sure. I wonder if the avatar, byline, name, and bio should all be their own blocks inside the Author block. There is possibly even some merit in being able to add those blocks on their own to get really creative with layout and presentation of the author. On the one hand, some things get simpler. To remove an avatar, just select it and delete it. To add it back, click the child block inserter. To edit a byline, just click into the field and start typing. To remove it, delete the content of the field. The responsibilities of the parent Author block would then be reduced to style variants and switching the author (maybe?). Where it gets complicated is how you structure the child blocks. Do you have a very rigid format that prevents the user from moving blocks around? This would enable the styles to be predictable and will have the user relying more on the style variants provided by the theme (if any). If it isn't done with a rigid inflexible layout, will the user have to use things like columns to layout the content inside the Author block? That adds one more layer of complexity. |
I had this same thought early on, using child blocks to define all the elements. But then that would make laying out all the elements more difficult, requiring things like a Columns block to align the profile photo to the left of the name and bio. It still might be worth a exploration, though. -- Here's another take on using toggle buttons in the toolbar for the profile photo and bio, with an "edit" pencil icon for changing the display name and author: I added a new interaction for changing the photo size, but it feels a little strange. |
Perhaps this thread is not the best place to discuss...but, has any consideration been given to how this block might interplay with https://wordpress.org/plugins/co-authors-plus/? |
@mdrovdahl, the co-authors option was explored in the issue. I think it's most likely a version 2 for this. @shaunandrews, I like the further explorations there! I wonder if we need to worry about an avatar size right now. Can the theme handle that instead? I imagine the border-radius would come from the theme as well. Overall, the avatar size option isn't too bad. It borders on being an inner-block at that point, but very limited, which I think is good. |
@mapk @shaunandrews I wonder if we could iterate on this block in future PRs? For now the block has the required basic features and most of the interactions suggested here would use the incoming G2. I can take these suggestions and make new issues out of them following this merge and move on with the current Inspector based interactions this PR implements. |
this aloows us to use the sizes as the API serves them
Thanks a ton for making those changes!
👍
Yeah, I definitely don't want to block this PR any further -- any refinements can be made in follow-up PRs. I'll give it a last round of testing and will then approve! |
const avatarSizes = []; | ||
if ( authorDetails ) { | ||
forEach( authorDetails.avatar_urls, ( url, size ) => { | ||
avatarSizes.push( { | ||
value: size, | ||
label: `${ size } x ${ size }`, | ||
} ); | ||
} ); | ||
} |
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 do
const avatarSizes = []; | |
if ( authorDetails ) { | |
forEach( authorDetails.avatar_urls, ( url, size ) => { | |
avatarSizes.push( { | |
value: size, | |
label: `${ size } x ${ size }`, | |
} ); | |
} ); | |
} | |
let avatarSizes = []; | |
if ( authorDetails ) { | |
avatarSizes = Object.keys( authorDetails.avatar_urls ).map( ( size ) => ( { | |
value: size, | |
label: `${ size } x ${ size }`, | |
} ) ); | |
} |
-- I personally prefer .map
over .forEach
since it's a bit more obvious to see that we're mapping one set of values to another.
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.
@ockham 's comment here was marked resolved, but I don't see there there were any revisions. I agree with the point to be using map
here. Lodash's map
can be slightly more expressive, at least since we're already using forEach
from Lodash anyways:
const avatarSizes = map( authorDetails?.avatar_urls, ( url, size ) => ( {
value: size,
label: `${ size } x ${ size }`,
} );
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.
Thanks, this is working nicely!
I left a few suggestions -- mostly syntactic sugar, none of them blocking.
Overall, this is good to merge! 🚢
if ( empty( $attributes ) ) { | ||
return ''; | ||
} |
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.
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.
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.
This is interesting. I don't remember what was the cause of that condition and it does look weird to me as well, not rendering anything.
On the side of FSE however, that is more nuanced. What you see is a placeholder that exists only if there is no author to show. If there is no author to show, how should the inspector controls behave? Considering they're mostly styling controls ...
[ authorId ] | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
const DEFAULT_AVATAR_SIZE = 24; |
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.
Is there a reason to assign this in code, rather than as the default value for the attribute?
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.
Good idea, it should be the default attr val.
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.
Good idea, it should be the default attr val.
I looked at it briefly as part of #22359, and there was one potential "gotcha" which could explain why it was treated differently.
At this line of code, we'll only set a value if there's one from attributes, and not consider the default:
value={ attributes.avatarSize } |
I can't say whether that was intentional, but I could see a case to avoid showing the default value as selected (i.e. instead, show nothing as selected until an explicit choice is made). This would behave a lot like the font size picker of Paragraph block, for example.
I still wish it were something we could do as a default attribute instead. Not only does it avoid duplication, it could probably avoid some bugs (e.g. the Post Author avatar currently won't show on the front-end if there is no size selected).
For me, it could be:
- Just show the selected value, even if it's the default
- Or, hide the value when it's the same as the default
- Note: This should consider if the user makes an explicit choice which coincidentally is the same as the default. Technically, these are treated as roughly equivalent, though probably not as expected (Dynamic blocks: default values for attributes aren't converted into html comments #7342).
- Or, implement some way to differentiate whether an attribute value was actually sourced from the block's original markup, or if it was provided as a default.
Description
Closes #19696
Aside from the implementation here and the details in the review, the follwing things are still pending:
Things to explore, possibly outside this PR based on feedback:
Screenshots
How has this been tested?