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

Enable Post Featured Image to be set, unset and replaced #27224

Merged
merged 3 commits into from
Nov 24, 2020

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Nov 24, 2020

Description

Resolves: #24813

This PR enables Post Featured Image block to add a Featured image if not exists, to unset it and replace it. If no Featured image exists it shows a Placeholder to set one.

Screenshots

featuredImage

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

-- Thank you @mapk for creating the icon so fast after asking :)!

@ntsekouras ntsekouras added [Feature] Full Site Editing [Type] Feature New feature to highlight in changelogs. [Block] Post Featured Image Affects the Post Featured Image Block labels Nov 24, 2020
@ntsekouras ntsekouras self-assigned this Nov 24, 2020
@github-actions
Copy link

github-actions bot commented Nov 24, 2020

Size Change: +294 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-directory/index.js 8.72 kB +2 B (0%)
build/block-editor/index.js 133 kB -5 B (0%)
build/block-library/editor-rtl.css 8.96 kB -1 B
build/block-library/editor.css 8.96 kB -1 B
build/block-library/index.js 148 kB +299 B (0%)
build/components/index.js 172 kB +54 B (0%)
build/dom/index.js 4.95 kB +35 B (0%)
build/edit-navigation/index.js 11.1 kB -36 B (0%)
build/edit-post/index.js 306 kB +55 B (0%)
build/edit-post/style-rtl.css 6.46 kB +2 B (0%)
build/edit-post/style.css 6.44 kB +3 B (0%)
build/edit-site/index.js 23.6 kB +6 B (0%)
build/edit-widgets/index.js 26.3 kB -113 B (0%)
build/editor/index.js 43.3 kB -5 B (0%)
build/reusable-blocks/index.js 2.92 kB +1 B
build/rich-text/index.js 13.3 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.8 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-site/style-rtl.css 3.86 kB 0 B
build/edit-site/style.css 3.86 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.81 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I haven't tested but the code itself looks good 👍

packages/block-library/src/post-featured-image/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/post-featured-image/edit.js Outdated Show resolved Hide resolved
Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

Code looks good and appears to be working fine on my test 👍
I agree that it does look weird to have a group for a single item as mentioned in the other comment by @gziolo but it doesn't interfere with anything so I'm OK with this.

>
{ __( 'Clear' ) }
</ToolbarButton>
<MediaReplaceFlow
Copy link
Member

Choose a reason for hiding this comment

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

One thing I noticed is that MediaReplaceFlow uses ToolbarGroup internally:

<ToolbarGroup className="media-replace-flow">

That puts a group in another group. It something that we should look into separately as it might be too restrictive choice as @jasmussen pointed out in the linked issue.

@ntsekouras ntsekouras merged commit 08d56e3 into master Nov 24, 2020
@ntsekouras ntsekouras deleted the add/post-feautered-image-controls branch November 24, 2020 14:11
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 24, 2020
@mapk
Copy link
Contributor

mapk commented Nov 24, 2020

I'm unclear about the "unset" feature on this block. Would a user ever unset the featured image and leave the block blank?

Unsetting, to me, feels much more like just removing the block. Otherwise the user would "replace" the image.

Also now that I see this in action, the unset icon needs iterating. It doesn't work visually next to the block icon.

@mapk
Copy link
Contributor

mapk commented Nov 24, 2020

Here's a visual how the flows currently work in relation to this PR.

flow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Featured Image Affects the Post Featured Image Block [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Featured Image Block: Add image from block
5 participants