Add AMP preview button #812
Conversation
| @@ -0,0 +1,22 @@ | |||
| /** | |||
There was a problem hiding this comment.
It seems overkill to enqueue a file for this small amount of CSS but it is done that way in preparation of #709.
| * @param {Object} data Object data. | ||
| * @return {void} | ||
| */ | ||
| boot: function( data ) { |
There was a problem hiding this comment.
It seems overkill to have a boot() method just to call addPreviewButton() method but it is structured that way in preparation of #709.
assets/css/amp-post-meta-box.css
Outdated
|
|
||
| /* AMP preview button */ | ||
| #preview-action.has-next-sibling .preview.amp-preview { | ||
| border-top-left-radius: 0; |
There was a problem hiding this comment.
Oops, thanks for picking that one up.
I just switch to Atom last week and I guess this is the first flaw I see. For those who use Atom here is what I see by default.
Tips: in settings->editor enable "Show invisible" and then you get
which is already an improvement. But better, install "Tabs to Space" package and set it on save which will automatically convert such inconvenience.
assets/js/amp-post-meta-box.js
Outdated
| * | ||
| * @since 0.6 | ||
| */ | ||
| var AmpPostMetaBox = ( function( $ ) { |
There was a problem hiding this comment.
Since this isn't a class, probably ampPostMetaBox would be a better name.
| /** | ||
| * Assets handle. | ||
| * | ||
| * @const string |
There was a problem hiding this comment.
I think @var is better here. See https://stackoverflow.com/a/18446766/93579
| * | ||
| * @since 0.6 | ||
| */ | ||
| public function enqueue_admin_assets() { |
There was a problem hiding this comment.
This is currently enqueueing the assets for every admin screen. It should be limited to just the edit post screen.
There was a problem hiding this comment.
I am not sure if I am missing something but from my perspective it is not loading on all pages since it checks if the $post->type is set and the post_supports_amp(). It does load on the edit.php, post.php and post-new.php if the post type as AMP support. That said it isn't really necessary to load it on edit.php so I will add a check to strictly load it on post.php and post-new.php.
There was a problem hiding this comment.
@ThierryA oh yes, I see you're right. and that it wouldn't get enqueued. But it would get enqueued if an admin screen happened to define a global $post and didn't clean it up. So yeah, I think it is best to explicitly check if the current_screen is the post editor. Maybe 'post' === get_current_screen()->base would be a good check?
| array( 'jquery' ), | ||
| AMP__VERSION | ||
| ); | ||
| wp_add_inline_script( self::ASSETS_HANDLE, sprintf( 'AmpPostMetaBox.boot( %s );', |
There was a problem hiding this comment.
I love this way of booting JS.
assets/js/amp-post-meta-box.js
Outdated
| 'href': this.data.previewLink, | ||
| 'id': 'amp-' + $previewBtn.prop( 'id' ), | ||
| 'target': 'amp-' + $previewBtn.prop( 'target' ) | ||
| } ) |
There was a problem hiding this comment.
This isn't working yet with autosave revisions. If you try clicking the AMP preview before an autosave happens, then the opened window will not show the changes you've made.
This logic needs to be incorporated: https://github.com/WordPress/wordpress-develop/blob/a825a181e11b1a89e76d79d22bd2c220f45b3741/src/wp-admin/js/post.js#L388-L418
There was a problem hiding this comment.
[updated] Discard previous comment, I need to do more work on this.
There was a problem hiding this comment.
@westonruter this commit address this issue, it should be working as expected now.
|
@westonruter kindly review the changes, this should be ready to merge. |
…eature/709-block-amp-per-page
…t skipped. * Restore focus on edit link for AMP status edit for accessibility. * Do not add AMP preview button if AMP has been disabled. * Change postmeta to be flag indicating whether AMP is disabled. * Fix spelling and clean up phpdoc.
…MP_Post_Template for sake of preview
|
Closing in favor of #813, as I got the two branches intertwined. |
This PR adds the AMP preview button on posts which have AMP support.
Refer to the ACs for more info...
Close #799