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: Post Author: Refactor edit to use block context #22359

Merged
merged 3 commits into from
May 14, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented May 14, 2020

Previously: #21696
Related: #19894

This pull request seeks to refactor the Post Author block to use block context within the editor implementation, replacing useEntityProp with equivalent entity selectors / action dispatches operating from context postId and postType values.

Implementation notes:

A few general refactoring changes have been applied to the Post Author block based on comments at #19894 (comment), #19894 (comment), #19894 (comment).

The two components of the file have been merged to one. There was previous discussion of this at #19894 (comment). This was done largely out of removing this early return condition. I do not believe it should ever be the case that a post would not have an author associated with it. There may be a delay in loading details about that author, but we should still be able to render most of the elements of the block (inspector controls, etc). A placeholder, if applicable, could ideally be limited to just substituting the block list elements. Perhaps this could be split again to another smaller component, if necessary.

Testing instructions:

Repeat Testing Instructions from #19894, verifying there are no regressions in behavior.

@github-actions
Copy link

github-actions bot commented May 14, 2020

Size Change: -113 B (0%)

Total Size: 832 kB

Filename Size Change
build/annotations/index.js 3.62 kB -3 B (0%)
build/block-editor/index.js 104 kB +67 B (0%)
build/block-library/editor-rtl.css 7.19 kB -67 B (0%)
build/block-library/editor.css 7.19 kB -66 B (0%)
build/block-library/index.js 118 kB +90 B (0%)
build/block-library/style-rtl.css 7.48 kB -5 B (0%)
build/block-library/style.css 7.48 kB -3 B (0%)
build/blocks/index.js 48.1 kB +4 B (0%)
build/components/index.js 182 kB -61 B (0%)
build/compose/index.js 6.67 kB -3 B (0%)
build/core-data/index.js 11.4 kB -1 B
build/data/index.js 8.42 kB -7 B (0%)
build/date/index.js 5.47 kB -3 B (0%)
build/edit-navigation/index.js 5.6 kB -5 B (0%)
build/edit-post/index.js 28.1 kB -73 B (0%)
build/edit-site/index.js 12 kB +9 B (0%)
build/edit-widgets/index.js 7.87 kB +7 B (0%)
build/editor/index.js 44.3 kB +13 B (0%)
build/element/index.js 4.65 kB +1 B
build/format-library/index.js 7.63 kB +5 B (0%)
build/hooks/index.js 2.13 kB -2 B (0%)
build/i18n/index.js 3.56 kB -1 B
build/is-shallow-equal/index.js 711 B +1 B
build/keyboard-shortcuts/index.js 2.51 kB -4 B (0%)
build/keycodes/index.js 1.94 kB -1 B
build/list-reusable-blocks/index.js 3.13 kB +1 B
build/media-utils/index.js 5.29 kB -4 B (0%)
build/notices/index.js 1.79 kB -1 B
build/nux/index.js 3.4 kB +1 B
build/plugins/index.js 2.56 kB +1 B
build/priority-queue/index.js 789 B +1 B
build/redux-routine/index.js 2.85 kB +1 B
build/rich-text/index.js 14.8 kB -2 B (0%)
build/shortcode/index.js 1.7 kB -1 B
build/token-list/index.js 1.28 kB +1 B
build/url/index.js 4.02 kB -2 B (0%)
build/warning/index.js 1.14 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.59 kB 0 B
build/block-directory/style-rtl.css 764 B 0 B
build/block-directory/style.css 764 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 618 B 0 B
build/edit-navigation/style.css 617 B 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@aduth
Copy link
Member Author

aduth commented May 14, 2020

The changes here illustrate an interesting difference in the patterns, where useEntityProp certainly does add a layer of convenience that we'd lose by using the underlying selectors and actions directly.

Roughly, I think it's the difference of:

Before:

function MyComponent() {
    const [ authorId, setAuthorId ] = useEntityProp( 'postType', 'post', 'author' );
}

After:

function MyComponent( props ) {
    const { postType, postId } = props.context;
    const authorId = useSelect(
        ( select ) => select( 'core' ).getEditedEntityRecord( 'postType', postType, postId )?.author ),
        [ postType, postId ]
    );
    const { editEntityRecord } = useDispatch( 'core' );
    const setAuthorId = ( nextAuthorId ) => editEntityRecord( 'postType', postType, postId, { author: nextAuthorId } );
}

(Note: The former doesn't support context, or non-post post types)

Part of me thinks that this is okay, and that it helps reaffirm and build consistent knowledge around selectors, action dispatchers, and the core data module in particular.

On the other hand, maybe we don't need to abandon this convenience? I'm not sure if it could be built in to useEntityProp, or would need to be something new. The main blocker would be that useEntityProp can only currently receive the type, but not the ID. It may be possible to overload it in such a way that it could optionally accept an ID as well.

@aduth aduth merged commit 7f89ccc into master May 14, 2020
@aduth aduth deleted the update/editor-block-context-post-author branch May 14, 2020 22:21
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 14, 2020
@aduth
Copy link
Member Author

aduth commented May 14, 2020

I do not believe it should ever be the case that a post would not have an author associated with it.

Only now after merging, I have the sudden realization that this fails to account for the Site Editor, where there is no post 😩

I'll follow-up with something shortly. It's not entirely broken, per se, but it does try to present as if it were the block of a specific post.

image

@aduth
Copy link
Member Author

aduth commented May 14, 2020

Maybe it's not worth worrying too much over right now. I don't know that restoring the text "Post Author Placeholder" is really any better 😄 If anything, this might be closer to what it should be in the case of a site editor representation of the block? As far as: Availability of block inspector controls, general markup structure (albeit likely stubbed with fake data or skeleton loader-esque styling).

@epiqueras
Copy link
Contributor

Only now after merging, I have the sudden realization that this fails to account for the Site Editor, where there is no post 😩

This is fixed by #22368.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants