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

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

Merged
merged 6 commits into from
May 18, 2021

Conversation

Tug
Copy link
Contributor

@Tug Tug commented May 7, 2021

This PR fixes an issue with the story block toolbar edit button that was reported in #19628 (comment)
More precisely, the render props of the MediaUpload component used in the story block's toolbar is being ignored and replaced with a button having the label "Select Media".

result

Changes proposed in this Pull Request:

This PR attempts to fix this issue by looking at the element rendered in MediaUpload. If it's a simple button containing only text then we can replace it with our own label, otherwise we try to reuse the rendered element. In this case we check if the element's children props is simply a string. There are other approaches we could take: for instance we could whitelist EditButton intances. I'm open to other ideas.
Most recent work on this component: #19644

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

no

Testing instructions:

  • Edit a post containing a story block using an account connected to WPCOM (so you have external media sources available such as Google Photos or Vexel)
  • Select the story block
  • Notice the pencil icon is used in the block toolbar to edit the media used for the story
  • Try inserting one image, then multiple media
  • Make sure the "pencil" button and the "Select Media" one are still working and and display the menu with different sources on click. Make sure selecting media from any sources work as expected

Screenshot 2021-05-13 at 15 24 44

Make sure there is no regressions:

  • Set and unset the post featured image on a site connected to and disconnected from WordPress.com

  • Image block: make sure the block edit button displays "Replace"

  • Gallery block: makes sure the default button for adding media displays "Select Images"

  • Gallery block: makes sure that for each image, the button to modify it displays "Select Image"

  • Slideshow block: the button for adding media should display "Select Images"

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello Tug! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D61141-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

@Tug Tug added [Block] Story [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels May 7, 2021
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label May 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

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.

@Tug Tug requested a review from aforcier May 10, 2021 12:26
@Tug Tug self-assigned this May 10, 2021
@Tug Tug marked this pull request as ready for review May 10, 2021 12:26
@aforcier
Copy link
Contributor

I gave this a try and it looks to have a few issues:

  1. When there is only one slide in the story, a 'Replace image' text button appears next to the pencil icon:

Screen Shot 2021-05-11 at 10 35 54 AM

  1. When clicking the pencil button (regardless of number of slides), the media already in the story are not pre-selected in the media library view, as they are when clicking the 'Select media' text button under the story

  2. Also in the above case, at least on my (Jetpack) site the media selection seems to be in 'feature image' mode - the title is "Featured image," only one media item can be selected, and the confirmation button says "Set featured image" instead of "Select":

Screen Shot 2021-05-11 at 10 38 15 AM

I guess maybe the workaround is also setting it in 'feature image' mode in some way?

@aforcier aforcier added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Team Review labels May 11, 2021
@Tug
Copy link
Contributor Author

Tug commented May 13, 2021

@aforcier Updated the PR and its description, this should be fixed now

@Tug Tug added [Status] Needs Team Review [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 13, 2021
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me. 🚢

@jeherve jeherve added this to the jetpack/9.8 milestone May 14, 2021
@jeherve jeherve removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label May 14, 2021
@samiff samiff added the [Status] Needs Testing We need to add this change to the testing call for this month's release label May 14, 2021
Copy link
Contributor

@aforcier aforcier left a comment

Choose a reason for hiding this comment

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

Tests well for me now, on both Jetpack and WPcom 👍

@Tug Tug merged commit 35f9f9f into master May 18, 2021
@Tug Tug deleted the story-block/fix/block-option-pencil-icon branch May 18, 2021 06:21
@github-actions
Copy link
Contributor

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

Thank you!

@Tug
Copy link
Contributor Author

Tug commented May 18, 2021

Deployed to WPCOM with changeset ID 225846

@samiff samiff removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label May 24, 2021
jeherve added a commit that referenced this pull request Mar 4, 2022
This was already refactored in #19778, but didn't need to remain that complicated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Story [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants