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

Editor: Support VideoPress post-editor preview (via Shortcode component) #8342

Merged
merged 4 commits into from
Oct 10, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Sep 29, 2016

Supersedes #3832, #2267
Closes #2267

This pull request seeks to implement VideoPress previews in the editor.

Videos

Implementation Notes:

The changes in this pull request are nearly identical to those of #3832, where the primary issue was lack of localStorage access because of sandboxing of the <Shortcode /> iframe (necessary for remembering the volume preference). This was remedied by removing the iframe sandboxing, but enforcing that in doing so, the shortcode result be processed by WordPress.com . In the case of VideoPress shortcodes, this should always be accurate since the result will be the same on WordPress.com as it would be on a Jetpack site.

Testing Instructions:

Verify that you can insert a VideoPress video in the editor for a WordPress.com or Jetpack site, and that in doing so the video preview is shown in the editor, sized to the videos natural ratio. Next ensure that the video continues to display correctly after saving and refreshing the page, or when toggling between HTML and Visual editing modes.

cc @mjuhasz @timmyc

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 29, 2016
@matticbot
Copy link
Contributor

@timmyc
Copy link
Contributor

timmyc commented Oct 3, 2016

This is testing out well for me. When uploading a new video, there were a couple of 404's noted in the console, but obviously this is something videopress related:

edit_post_ trout_bummin _wordpress_com

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 3, 2016
@rralian
Copy link
Contributor

rralian commented Oct 5, 2016

I had a little glitch the first time I tested it, where adding a video, letting it load, then toggling into the html view and then back left me with no loaded video, and just a long blinking line to the left of the where the video was supposed to show up. But then after a reload and several tried I was unable to reproduce it. So I second the motion to merge. :shipit:

@timmyc
Copy link
Contributor

timmyc commented Oct 5, 2016

just a long blinking line to the left of the where the video was supposed to show up

I did hit this once also - but was unable to consistently reproduce.

@aduth
Copy link
Contributor Author

aduth commented Oct 5, 2016

I was already a bit hesitant about allowing some escape from <Shortcode /> sandboxing, then had the realization that while I had applied ?force=wpcom on the request if modifying the sandbox attribute, if for some reason the shortcode result already existed in the Flux store and had been requested without the WP.com restriction, we'd still render it without the sandbox attribute. 🙀

The whole point of this was to get around the issue of localStorage not being available for sandboxed iframes, which is used for persisting the volume in the VideoPress embed. It seems fine enough to me from a UX point-of-view that this not be persisted in the preview, though the errors themselves are a nuisance.

I've separately opened pull requests to enhance the VideoPress embed to accept an argument to avoid attempting to persist the volume (6-gh-videopress-routes, 18-gh-videopress). Would it be agreeable to move forward with this despite access errors being logged from the VideoPress script? I've re-added the sandbox restrictions in a2cc3e7.

@aduth aduth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Ready to Merge labels Oct 5, 2016
@mjuhasz
Copy link
Contributor

mjuhasz commented Oct 5, 2016

This branch works great (if run locally)! Considering the current user experience, it would be a huge improvement to move forward with this branch, despite the volume not persisted.

For some reason, this branch does not work on calypso.live, here are the screenshots on Chrome and Firefox, respectively:

chrome

firefox

@aduth
Copy link
Contributor Author

aduth commented Oct 6, 2016

@mjuhasz Nice catch. Appears calypso.live wasn't a whitelisted origin for previews. This is fixed in r143314-wpcom .

@lancewillett
Copy link
Contributor

Does this change need more review, or is it ready for going live now?

@aduth aduth force-pushed the add/editor-video-view branch from a2cc3e7 to a5c6040 Compare October 10, 2016 18:17
@aduth aduth force-pushed the add/editor-video-view branch from a5c6040 to f8a6d50 Compare October 10, 2016 18:22
@aduth
Copy link
Contributor Author

aduth commented Oct 10, 2016

Let's get it out there. Did a last rebase and final round of browser support. Merging now.

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 10, 2016
@aduth aduth merged commit 466e6b2 into master Oct 10, 2016
@aduth aduth deleted the add/editor-video-view branch October 10, 2016 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants