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

Created author block. #3250

Closed
wants to merge 1 commit into from
Closed

Created author block. #3250

wants to merge 1 commit into from

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Oct 30, 2017

Description

This PR aims to add an author block, as specified in issue #1844..

Screenshots

screen shot 2017-11-01 at 13 32 12

screen shot 2017-11-01 at 13 32 01

screen shot 2017-11-01 at 13 32 24

screen shot 2017-11-01 at 13 32 42

screen shot 2017-11-01 at 13 32 51

screen shot 2017-11-01 at 13 33 18

screen shot 2017-11-01 at 13 33 30

screen shot 2017-11-01 at 13 33 38

To test

Add an author block, try to hide each of three options see the hiding attribute works as expected in the editor and when saving the post.
Try each of the alignment options verify the alignment looks ok.

Notes

The single-author component is going to be reused in author blocks.
A max-width of 50% was added when align is not center, because without the post after published does not look as in the editor and in fact, no difference is noticeable when compared to center alignment.

@jorgefilipecosta jorgefilipecosta added [Feature] Blocks Overall functionality of blocks New Block Suggestion for a new block [Status] In Progress Tracking issues with work in progress [Type] Task Issues or PRs that have been broken down into an individual action to take labels Oct 30, 2017
@jorgefilipecosta jorgefilipecosta self-assigned this Oct 30, 2017
@jorgefilipecosta jorgefilipecosta force-pushed the add/author-block branch 4 times, most recently from a9e4cc2 to 24ceba1 Compare November 1, 2017 13:46
@codecov
Copy link

codecov bot commented Nov 1, 2017

Codecov Report

Merging #3250 into master will decrease coverage by 7.47%.
The diff coverage is 20.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3250      +/-   ##
==========================================
- Coverage   38.49%   31.01%   -7.48%     
==========================================
  Files         286      235      -51     
  Lines        6942     6545     -397     
  Branches     1273     1164     -109     
==========================================
- Hits         2672     2030     -642     
- Misses       3590     3788     +198     
- Partials      680      727      +47
Impacted Files Coverage Δ
blocks/single-author/index.js 0% <0%> (ø)
editor/selectors.js 94.41% <100%> (+0.91%) ⬆️
blocks/library/author/block.js 11.76% <11.76%> (ø)
blocks/library/author/index.js 40% <40%> (ø)
components/notice/index.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
components/keyboard-shortcuts/index.js 0% <0%> (-85.72%) ⬇️
components/slot-fill/provider.js 5% <0%> (-85.33%) ⬇️
... and 304 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71a218f...cd9423a. Read the comment docs.

@jorgefilipecosta jorgefilipecosta added Needs Design Feedback Needs general design feedback. and removed [Status] In Progress Tracking issues with work in progress labels Nov 1, 2017
@jorgefilipecosta jorgefilipecosta changed the title WIP - Created author block. Created author block. Nov 1, 2017
@karmatosed
Copy link
Member

As you came so far with this, want to fix up the issues and we can review @jorgefilipecosta ?

@jorgefilipecosta
Copy link
Member Author

Hi @karmatosed, I rebased this, the automated tests were failing because of a change in our mechanism. I think this is ready for a review/testing.


category: 'widgets',

keywords: [ __( 'author' ) ],
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to define this keyword, since inserter search already uses the "title" field for search, and it's unlikely "Author" would be translated differently than "author".

} )
),
withAPIData( ( props ) => ( {
author: `/wp/v2/users/${ props.postAuthorId }`,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a complex problem, that probably will be faced in other blocks.
I'm seeing two possible paths:
We conclude that this information should be available in the API and we create a patch for that. To me author name, description and avatar look like information that can be public.
We conclude that this information should not be public. In this case, we need to have a mechanism to only allow the author of the post to add this block. And we need a state that shows some message like "No permissions to display this block in the editor, please use the preview to visualize this block content" when the post is being edited by a user that is not admin or post author.

*/
function gutenberg_render_block_core_author( $attributes ) {
$align = 'center';
if ( isset( $attributes['align'] ) && in_array( $attributes['align'], array( 'left', 'right', 'full' ), true ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

The isset here may be unnecessary, since we'd always assume it to be set.

text-align: center;
}
p:after {
content: "";
Copy link
Member

Choose a reason for hiding this comment

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

Tabbed too far here.

"name": "core/author",
"isValid": true,
"attributes": {
"hideBio": false,
Copy link
Member

Choose a reason for hiding this comment

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

Wonder why tabbing is off here, since this is generated. 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Here in github it looks like it has wrong tabbing but I checked in the editor the tabs were right, looks like this is related with github visualization.

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 a difference of tabs and spaces. Depending on how your editor is configured, it might be showing tabs as 4 spaces, appearing aligned.

@@ -0,0 +1 @@
<!-- wp:core/author {"hideBio":true,"hideAvatar":true,"hideName":true} /-->
Copy link
Member

Choose a reason for hiding this comment

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

The wp:core/ is no longer necessary, and can be seen as stripped in the serialized version.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed wp:core/author to wp:author, but unfortunately, some of our PHP tests started failing (possible to see in our CI). I probably misunderstood the change :(

* @param {Object} state Global application state
* @return {Number} ID of post author
*/
export function getCurrentPostAuthor( state ) {
Copy link
Member

Choose a reason for hiding this comment

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

Opens a question as to whether we want granular selectors for every single property of a post, or if it's reasonable to expect a developer to use getCurrentPost( state ).author directory (or alternatively, a getCurrentPostAttribute( state, key ) selector?).


keywords: [ __( 'author' ) ],

supportHTML: false,
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be rebased per #4014

@gziolo gziolo removed their request for review January 27, 2018 18:39
@karmatosed
Copy link
Member

Can we rebase this? Just so we can review.

export default compose(
connect(
( state ) => ( {
postAuthorId: getEditedPostAttribute( state, 'author' ),
Copy link
Member

Choose a reason for hiding this comment

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

See also https://github.com/WordPress/gutenberg/pull/5602/files#r175522235 for a proposal where I think we could formalize the dependence on this post context via a new attribute source type.

@karmatosed
Copy link
Member

Do we still want this open or close?

@jorgefilipecosta
Copy link
Member Author

Sorry, @karmatosed missed your ping. I think this PR is not relevant I will close. During next phases, we can reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. New Block Suggestion for a new block [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants