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

External Media: Updates for 8.7 #16069

Merged
merged 11 commits into from
Jun 25, 2020
Merged

External Media: Updates for 8.7 #16069

merged 11 commits into from
Jun 25, 2020

Conversation

obenland
Copy link
Member

@obenland obenland commented Jun 4, 2020

Changes in this PR:

Changes proposed in this Pull Request:

  • Adds a handler for 401 responses of the list endpoint.
  • Adds margins for "Select Image" button in placeholders and the featured image panel
  • Re-renders Google media modal after authentication instead of trying to fill it with a new query
  • Updates button test to better reflect accepted media types
  • Handles focus updates from arrow key presses
  • Adds API unit tests and updates /copy permissions check.
  • Updated image insert flow.
  • Removes external media option from existing galleries.

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

No.

Testing instructions:

It might be easiest to review the merged PRs separately, including their testing instructions.
Generally, these steps should cover most if not all of what was changes here:

  • Open the editor and insert the following blocks: image, media/text, gallery, cover
  • Notice how the insert button text is appropriate for the types of media they accept
  • Notice that there are correct margins (including featured images panel)
  • Insert images into the gallery from one of the external libraries.
  • Notice the new insert flow that keeps the full modal open and shows it as busy
  • If you chose Google Photos, notice how the modal loads recent photos after authentication (not albums)
  • After inserting images, notice that external libraries are no longer available to add more images to the gallery

Proposed changelog entry for your changes:

  • Introducing: External Media!

@obenland obenland added [Status] In Progress [Type] Task [Extension] External Media Extend all block editor media tools to support external providers labels Jun 4, 2020
@obenland obenland added this to the 8.7 milestone Jun 4, 2020
@jetpackbot
Copy link

jetpackbot commented Jun 4, 2020

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16069

Generated by 🚫 dangerJS against d2c4f68

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello obenland! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D44788-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@obenland
Copy link
Member Author

@jeherve @kraftbj Do you think I could elicit an initial review from one of you, despite this missing an Ajax ✅ ?
All changes in this PR received a team review prior to having been merged.

It would help me tremendously in giving me more time to respond to any change requests you may have.

@kraftbj kraftbj added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jun 18, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Jun 18, 2020

I'm logging off right now but absolutely!

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 looks good to me and tests well for me. I only tested with Jetpack and Core blocks in WP 5.4. It may be worth testing with WP 5.3, and maybe with other image blocks from other plugins such as CoBlocks. I'll do a second pass next week if no one beats me to it.

I only noted some small layout issues with the "Select Images" button on the tiled gallery and slideshow blocks on WP 5.4 sans Gutenberg:

image

image

@marekhrabe
Copy link
Contributor

I have identified a compatibility issue with WP 5.3. We are using useBlockEditContext which is only available in 5.4 and newer. Its predecesor withBlockEditContext is safely available in 5.3 but on the contrary, it has just been removed from Gutenberg, meaning we cannot simply switch to the older way because it will be gone in a few versions. I'll figure out some abstraction that combines both ways so we can use it in Jetpack.

@obenland
Copy link
Member Author

@marekhrabe @jeherve I can't find a reference to useBlockEditContext in this PR. Can you help me understand what (if anything) needs to be done here?

@obenland
Copy link
Member Author

@jeherve 060b4f8 fixes the visual bug you encountered by removing access to external media library when these blocks contain images already. This brings the behavior en par with the gallery block.

@marekhrabe
Copy link
Contributor

I can't find a reference to useBlockEditContext in this PR. Can you help me understand what (if anything) needs to be done here?

It's already in master and has now been detected in this PR because we've decided to test with the older WP.

@retrofox
Copy link
Contributor

I can't find a reference to useBlockEditContext in this PR. Can you help me understand what (if anything) needs to be done here?

It's already in master and has now been detected in this PR because we've decided to test with the older WP.

Correct, but the issue is not being introduced by this PR, does it? It's already in the primary branch. The useBlockEditContext hook is imported by videopress and cover blocks.

obenland and others added 7 commits June 24, 2020 14:17
Co-authored-by: Marek Hrabe <marekhrabe@me.com>
* [not verified] Add External media endpoint

* [not verified] Add unit tests

* [not verified] Add class coverage info

* Simplify schema

* Fix image copy test

* Use local image

* Remove duplicate file ending

Props @frontdevde.
* [not verified] # This is a combination of 5 commits.

External Media: Improve Insert Flow

See #15867

Remove unused import

Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

Announce Inserting action along with alt text of selected images.

Update modal headers to reflect status

Props @jancavan.

* External Media: Fix mobile modal size (#16098)

* External Media: Improve Insert Flow

See #15867

* Remove unused import

* Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

* Announce Inserting action along with alt text of selected images.

* Fix mobile mobile width to take up the full available modal space

* Change is-copying to modifier instead of element to follow BEM syntax

Co-authored-by: Konstantin Obenland <obenland@gmx.de>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>

* External Media: Loading and disabled states for the copying state (#16103)

* disable pagination while loading

* fade out all images when copying

* disable event handlers and mark as disabled when copying

* disable pexels search when copying

* disabled google filters when copying

* add text to copying indicator

* disable google photos view selector when copying

* [not verified] remove debugging condition

* Pass functions not booleans to event emitters

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

* Removed :not() copying css state since they are the same modal now

* Reset focus to the modal after clicking Insert. Otherwise focus is reset to the body and focus needs to stay trapped within the modal.

* Cosmetically rearranging code order

* External Media: Screen Reader accessible states for Insert flow (#16113)

* Add visually hidden description for the external media modal to give improved screen reader instructions and communicate the copying state

* [not verified] Remove title attribute from grid images

* [not verified] Move Inserting Image... copy to before the image so it is announced first.

* Separate-out description and element id

Helps with keeping describedby id in sync

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>
Co-authored-by: Marek Hrabe <marekhrabe@me.com>
WunderBart and others added 4 commits June 24, 2020 14:19
* External Media: Disable for existing galleries
* Update comment
`addToGallery` is not used by all gallery-type blocks, like tiled-gallery or slideshow.

Props @jeherve.
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 25, 2020
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! I'd vote for us to merge this now as is, make a new call for testing, and keep iterating / fixing bugs in that feature branch if needed.

@retrofox
Copy link
Contributor

retrofox commented Jun 25, 2020

minor issues / not blockers

  • margin extends when clicking on the button (Maywood)

externa-media-magin-resize

@retrofox
Copy link
Contributor

Initially, the gallery block doesn't offer the external media button.

Screen Shot 2020-06-25 at 9 15 14 AM

Guessing it isn't intentional, considering #16176

@retrofox
Copy link
Contributor

It seems that these changes have been lost.

@retrofox
Copy link
Contributor

Initially, the gallery block doesn't offer the external media button.
Guessing it isn't intentional, considering #16176

The condition ! ( gallery && value ) defined here is FALSE for empty gallery blocks.

@getdave
Copy link
Contributor

getdave commented Jun 25, 2020

Would you find any value in e2e tests that could verify this entire feature? It's so large that testing and re-testing is very difficult. Happy to help if that's feasible although I'm not familiar with Jetpack testing at this point.

@obenland obenland merged commit ff68063 into master Jun 25, 2020
@obenland obenland deleted the feature/external-media branch June 25, 2020 17:11
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jun 25, 2020
@obenland
Copy link
Member Author

r209619-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Extension] External Media Extend all block editor media tools to support external providers Touches WP.com Files [Type] Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants