External Media: don't show duplicate buttons for post thumbnails #19644

mattwiebe · 2021-04-23T20:56:43Z

Previously, our "Select Image" button would still show alongside Core's "Replace Image" button. Now, Cores' button is not shown, and our button shows as "Replace Image" when an image has been selected.

Fixes #19555

Changes proposed in this Pull Request:

Post Thumbnail UI should no longer show duplicate buttons, and buttons shown are appropriate

Jetpack product discussion

n/a

Does this pull request change what data or activity we track or use?

no

Testing instructions:

apply soon-to-be-generated Diff to your WPCom sandbox
Open an existing post (or create a new one)
Open the "Featured Image" section in the Post sidebar
No featured image selected should look like:

* With a selected featured image:

Note that we use the Media Button in several locations throughout Gutenberg. These modifications should be isolated to featured image usage, but smoke testing other image-based blocks/settings since I can't find any tests for this.

matticbot · 2021-04-23T20:56:49Z

Caution: This PR has changes that must be merged to WordPress.com
Hello mattwiebe! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D60595-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2021-04-23T20:58:25Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: June 1, 2021.
Scheduled code freeze: May 24, 2021.

creativecoder · 2021-04-27T21:31:48Z

@mattwiebe I pushed a change in a327880 (feel free to discard if you don't agree).

It didn't seem necessary to add a button when there's no featured image set, so I turned the component into the dropdown trigger

creativecoder · 2021-04-27T22:17:39Z

Found an annoying issue I though we should fix, while we're at it: the media sources dropdown pops back open after you select an external media item. 78b57b5 fixes this by closing the dropdown menu when an item is clicked.

creativecoder

This is working as expected for me:

The featured image sidebar controls are now unchanged by the external media extension:

Without Jetpack	With Jetpack

I spot checked the Media and Media & Text blocks and did not see any regressions.

creativecoder · 2021-04-28T19:20:55Z

projects/plugins/jetpack/extensions/shared/external-media/media-button/media-menu.js

-	}
+	let isPrimary = isFeatured;
+	let isTertiary = ! isFeatured;
+	const extraProps = {};


Nitpick: maybe this should just be let isSecondary = false;? I'm not sure I see an advantage to using an object here (unless all the is* props here are added to it)

I enjoy nitpicks, and I would overall prefer that, too.

My reasoning is that I was trying to keep the surface area of changes as limited as possible, given the broad and varied use of media buttons throughout different parts of the Gutenberg UI. I wasn't confident that always adding the isSecondary prop (even set to false in all non-featured image uses) wouldn't disrupt those other uses.

Now, with a bit more distance, I can see that it will be fine. The prop is false by default in the Button component and will be in all situations save the "Replace Image" use here. I'll update it that way.

mattwiebe · 2021-04-29T15:26:06Z

@creativecoder Thanks for the review and improving the PR to boot! I took your nitpick above and appreciate it.

jeherve

This tests well for me. 🚢

Previously, our "Select Image" button would still show alongside Core's "Replace Image" button. Now, Cores' button is not shown, and our button shows as "Replace Image" when an image has been selected. Fixes #19555

creativecoder

Confirming this still works after rebase, testing external media extension:

Featured image adding and removing
Image block, including replace image flow
Media and Text block

github-actions · 2021-05-04T19:31:35Z

Great news! One last step: head over to your WordPress.com diff, D60595-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

matticbot added the Touches WP.com Files label Apr 23, 2021

mattwiebe added [Extension] External Media Extend all block editor media tools to support external providers [Status] Needs Team Review labels Apr 23, 2021

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Apr 23, 2021

mattwiebe requested review from a team April 23, 2021 22:00

creativecoder previously approved these changes Apr 27, 2021

View reviewed changes

creativecoder reviewed Apr 28, 2021

View reviewed changes

mattwiebe dismissed creativecoder’s stale review via e4591b0 April 29, 2021 15:23

mattwiebe added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Apr 29, 2021

jeherve assigned mattwiebe Apr 30, 2021

jeherve added this to the jetpack/9.8 milestone Apr 30, 2021

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Bug When a feature is broken and / or not performing as intended and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 30, 2021

jeherve previously approved these changes Apr 30, 2021

View reviewed changes

mattwiebe and others added 4 commits May 4, 2021 11:33

[not verified] Media Buttons: don't show duplicate buttons

5ac595a

Previously, our "Select Image" button would still show alongside Core's "Replace Image" button. Now, Cores' button is not shown, and our button shows as "Replace Image" when an image has been selected. Fixes #19555

[not verified] Wrap the featured image component when no image is set

cfd0ba0

[not verified] Closes media sources menu on click

d69e9e4

[not verified] ditch extraProps, just use isSecondary straight up

5f10a45

mattwiebe dismissed jeherve’s stale review via 5f10a45 May 4, 2021 16:33

mattwiebe force-pushed the fix/remove-replace-button branch from e4591b0 to 5f10a45 Compare May 4, 2021 16:33

kraftbj approved these changes May 4, 2021

View reviewed changes

creativecoder approved these changes May 4, 2021

View reviewed changes

mattwiebe merged commit e56cffa into master May 4, 2021

mattwiebe deleted the fix/remove-replace-button branch May 4, 2021 19:30

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 4, 2021

Tug mentioned this pull request May 7, 2021

Story Block: Fix edit button used in block's toolbar #19778

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External Media: don't show duplicate buttons for post thumbnails #19644

External Media: don't show duplicate buttons for post thumbnails #19644

mattwiebe commented Apr 23, 2021

matticbot commented Apr 23, 2021

github-actions bot commented Apr 23, 2021 •

edited

Loading

creativecoder commented Apr 27, 2021

creativecoder commented Apr 27, 2021

creativecoder left a comment

creativecoder Apr 28, 2021

mattwiebe Apr 29, 2021

mattwiebe commented Apr 29, 2021

jeherve left a comment

creativecoder left a comment

github-actions bot commented May 4, 2021

External Media: don't show duplicate buttons for post thumbnails #19644

External Media: don't show duplicate buttons for post thumbnails #19644

Conversation

mattwiebe commented Apr 23, 2021

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

matticbot commented Apr 23, 2021

github-actions bot commented Apr 23, 2021 • edited Loading

creativecoder commented Apr 27, 2021

creativecoder commented Apr 27, 2021

creativecoder left a comment

Choose a reason for hiding this comment

creativecoder Apr 28, 2021

Choose a reason for hiding this comment

mattwiebe Apr 29, 2021

Choose a reason for hiding this comment

mattwiebe commented Apr 29, 2021

jeherve left a comment

Choose a reason for hiding this comment

creativecoder left a comment

Choose a reason for hiding this comment

github-actions bot commented May 4, 2021

github-actions bot commented Apr 23, 2021 •

edited

Loading