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

Block Library: enhance the author block #19894

Merged
merged 6 commits into from Apr 30, 2020
Merged

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented Jan 25, 2020

Description

Closes #19696

Aside from the implementation here and the details in the review, the follwing things are still pending:

  • always use the display name
  • add font size to the inspector
  • add link option in inspector

Things to explore, possibly outside this PR based on feedback:

  • it is not obvious that changing the author in the block updates the post
  • inline changing of the author
  • is byline really needed? It could be a paragraph before it.
  • multiple authors in the block for one post

Screenshots

author-block

How has this been tested?

  1. Enable the full site editing experiment
  2. Create a single template for full site editing
  3. Create a post
  4. Add the author block
  5. Preview the post

@draganescu draganescu changed the title Enhances the simple author block Block Library: enhance the simple author block Jan 27, 2020
@draganescu draganescu changed the title Block Library: enhance the simple author block Block Library: enhance the author block Jan 29, 2020
@draganescu draganescu marked this pull request as ready for review January 31, 2020 16:16
@draganescu
Copy link
Contributor Author

@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?) 😁

@draganescu draganescu force-pushed the add/new-block-author branch 2 times, most recently from b1d46cf to 8edda56 Compare February 1, 2020 13:54
@draganescu draganescu added [Feature] Full Site Editing New Block Suggestion for a new block labels Feb 1, 2020
@draganescu draganescu added this to Needs review in Full site editing Feb 1, 2020
Copy link
Contributor

@tellthemachines tellthemachines left a 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:

Screen Shot 2020-02-04 at 3 38 51 pm

Screen Shot 2020-02-04 at 3 38 14 pm

packages/block-library/src/post-author/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/post-author/editor.scss Outdated Show resolved Hide resolved
@mapk mapk added the Needs Design Feedback Needs general design feedback. label Feb 5, 2020
@mapk
Copy link
Contributor

mapk commented Feb 5, 2020

Thanks for wrangling this PR, @draganescu! After fiddling with the block a bit, here's some feedback.

  1. The Avatar size is a bit out of context when I don't toggle "on" the avatar. Let's only show the avatar size if the avatar toggle is "on". Something like this:

showavatar 2020-02-05 09_21_23

  1. I have my display name set as "Mapk" so when I toggle "on" the "Show display name" option, I expected to see that, but instead saw my first and last name. When I toggled "off" it showed my display name. This felt like a bug.

  2. There is a lag when the page is loading. The block shows "Post Author Placeholder" instead of just showing the author's name.

lag 2020-02-05 09_12_11

  1. The byline, when added, should align where the Author name was before.

Screen Shot 2020-02-05 at 9 38 41 AM

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.

quote 2020-02-05 09_41_20

  1. The vertical and horizontal spacing should all be equal in these areas.

Screen Shot 2020-02-05 at 9 43 28 AM

Reducing the spacing to 10px helps add a consistent grid throughout. See below:

Screen Shot 2020-02-05 at 9 46 50 AM

@mapk
Copy link
Contributor

mapk commented Feb 5, 2020

When thinking about the positioning of the byline, I think it might work better aligned with the other text. Maybe even styled to look like the Quote block's citation.

Screen Shot 2020-02-05 at 9 50 39 AM

@draganescu draganescu force-pushed the add/new-block-author branch 2 times, most recently from f74b10f to 7565b18 Compare February 11, 2020 13:25
@draganescu
Copy link
Contributor Author

Current state:

Screenshot 2020-02-11 at 16 25 27

@shaunandrews
Copy link
Contributor

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:

Author Block i2

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.

@mapk
Copy link
Contributor

mapk commented Feb 12, 2020

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:

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?

Also, having the byline on it's own line seems a little much.

Your byline works beautifully.

@MichaelArestad
Copy link
Contributor

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.

@mapk
Copy link
Contributor

mapk commented Feb 13, 2020

What about an actual icon in the toolbar that toggles the avatar?

avatar 2020-02-13 12_45_26

I'm still iffy on how the avatar sizing might work though.

@shaunandrews
Copy link
Contributor

I wonder if the avatar, byline, name, and bio should all be their own blocks inside the Author block.

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:

author block

I added a new interaction for changing the photo size, but it feels a little strange.

@mdrovdahl
Copy link

mdrovdahl commented Feb 17, 2020

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/?

@mapk
Copy link
Contributor

mapk commented Feb 17, 2020

@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.

@draganescu
Copy link
Contributor Author

@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
@ockham
Copy link
Contributor

ockham commented Apr 29, 2020

Howdy @ockham I pushed a refactoring that removes all storage of author data from the block and leaves only the settings of the block. Also we use the entity save function (great idea! 🙇 ) to update the post when the author is changed (the post has to be saved).

Thanks a ton for making those changes!

Also I have updated the rendering of the block to not use attributes for author data but instead normal author meta WordPress functions.

👍

As for UI I say we merge it like this (UI-wise) and I am sure we can have a future iteration based on some design update. The date block is more fit to that in place style, this one not so (with all those settings). What do you think? We can always add a needs design feedback label, but I wish we could just merge the block since I am sure once used new ideas will appear.

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!

Comment on lines +81 to +89
const avatarSizes = [];
if ( authorDetails ) {
forEach( authorDetails.avatar_urls, ( url, size ) => {
avatarSizes.push( {
value: size,
label: `${ size } x ${ size }`,
} );
} );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do

Suggested change
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.

Copy link
Member

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 }`,
} );

Copy link
Contributor

@ockham ockham left a 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! 🚢

@draganescu draganescu merged commit 2c69473 into master Apr 30, 2020
@draganescu draganescu deleted the add/new-block-author branch April 30, 2020 11:13
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone Apr 30, 2020
@mapk
Copy link
Contributor

mapk commented Apr 30, 2020

Darn, I was hoping to catch this before merging. But I noticed the "byline" section is just too close to the Author's name.

Twenty Twenty

Screen Shot 2020-04-30 at 11 27 31 AM

Twenty Nineteen

Screen Shot 2020-04-30 at 11 28 02 AM

@MichaelArestad MichaelArestad moved this from Needs review to Done in Full site editing Apr 30, 2020
Comment on lines +98 to +100
if ( empty( $attributes ) ) {
return '';
}
Copy link
Member

Choose a reason for hiding this comment

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

In testing #22114 and rebasing #21696, I'm a bit confused by this condition. A newly inserted Post Author block will have no attributes. But don't we still want to show something for the front-end rendering for a block in this default state?

Copy link
Member

Choose a reason for hiding this comment

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

It's also more obviously a problem in the Site Editor, where none of the Block Inspector fields appear to display for the block to be able to modify the attributes. Is this expected?

image

Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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
  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Block Suggestion for a new block
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

New Block: Post Author