-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Updated: Pages: Include avatar in Author field. #63142
Updated: Pages: Include avatar in Author field. #63142
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +134 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Nice, this is working well on Grid and Table layouts. Can we also include it in the List layout? 🤞 |
What if I want to turn off the avatars? I've wondered a few times how we could have entries that are composed of multiple fields/values. Another example of this is date and time (I may want to remove the time). Maybe we need a sub-field concept? Or a format concept? For example I could configure a date format, and for authors you could add options for |
That's a valid feature, but potentially something that will get covered in #58012. I don't know if it needs to hold up this PR? |
> | ||
<img | ||
onLoad={ () => setIsImageLoaded( true ) } | ||
alt="" |
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.
Why is the alt text empty?
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.
Hi @carolinan good question, it was already empty before in this PR I'm just moving code from the previous AuthorField component https://github.com/WordPress/gutenberg/pull/63142/files#diff-4bc1dbbf462da7ed7ab2b76d40ee01a0a2906482b12bea67383ecbb5697176acL119. But I think it should have an alt text at least something simple as "Author Text" is better than nothing, I made that update.
@@ -395,6 +396,11 @@ export default function PostsList( { postType } ) { | |||
value: id, | |||
label: name, | |||
} ) ) || [], | |||
render: ( { item } ) => { | |||
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.
I don't think we should reuse this render field function. Posts list is meant to be reused by every post type eventually, have an API to get the fields by postType
, but I see templates a bit different than the 'normal' post types. Maybe I'm wrong, but it wouldn't hurt to duplicate just some code for now.
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.
Hi @ntsekouras, I applied your suggestion and did not reuse the field from the templates.
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 think this use case will be handled when we implement the combination of fields, but it's fine to move forwards with this one for now.
8add9e0
to
8730110
Compare
Yes, when we have field combination I can do a small refactor to use that, but I guess for now we can merge this one. |
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.
Left a couple of nits, but this looks good. Thank you!
}, | ||
[ item ] | ||
); | ||
const withIcon = viewType !== LAYOUT_LIST; |
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.
Nit: let's rename to withAvatar
for more clarity. Additionally and also nit is to combine the checks like ` {withAvatar && ( imageUrl ? ... : ) }
@@ -1,7 +1,16 @@ | |||
/** |
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.
Unrelated with this PR but this file should be moved to its own components/post-list folder or something, it's not specific to "posts app"
fc71ee8
to
aabc47d
Compare
@jorgefilipecosta it looks like the List layout was missed here. I only see the avatar in the Table or Grid layout. |
@jorgefilipecosta Also, I appreciate this is separate, but in the Templates table the author avatar appears in the Table layout, but not the List layout. It would be good to align across. |
Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: ellatrix <[email protected]>
Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: ellatrix <[email protected]>
Hi @jameskoster with the iterations we did there are avatars, on list, grid, and table on templates and pages. So everything you pointed out should be fixed. Let me know if there is anything related remaining. |
This PR includes the author avatar on the author field.
Part of: #63128
cc: @jameskoster
Screenshots
Testing Instructions
Assign an email with a gravatar image to one of the other of a page.
Load the pages list and verify the avatar is correctly shown.