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

Relocate AMP Preview Button #7914

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import PropTypes from 'prop-types';
/**
* WordPress dependencies
*/
import { Button, Icon, VisuallyHidden } from '@wordpress/components';
import { PluginPreviewMenuItem } from '@wordpress/editor';
import { Icon, VisuallyHidden } from '@wordpress/components';
import { compose } from '@wordpress/compose';
import { withDispatch, withSelect } from '@wordpress/data';
import { Component, createRef, renderToString } from '@wordpress/element';
Expand All @@ -16,7 +17,7 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import { isAMPEnabled } from '../helpers';
import { AMPFilledIcon, AMPBlackIcon } from '../../icons';
import { AMPBlackIcon } from '../../icons';

/**
* Writes the message and graphic in the new preview window that was opened.
Expand Down Expand Up @@ -95,11 +96,10 @@ function writeInterstitialMessage(targetDocument) {
}

/**
* A 'Preview AMP' button, forked from the Core 'Preview' button: <PostPreviewButton>.
* A 'Preview AMP' Menu Item.
*
* @see https://github.com/WordPress/gutenberg/blob/95e769df1f82f6b0ef587d81af65dd2f48cd1c38/packages/editor/src/components/post-preview-button/index.js#L95-L200
*/
class AmpPreviewButton extends Component {
class AmpPreviewMenuItem extends Component {
/**
* Constructs the class.
*
Expand All @@ -108,7 +108,7 @@ class AmpPreviewButton extends Component {
constructor(...args) {
super(...args);

this.buttonRef = createRef();
this.itemRef = createRef();
this.openPreviewWindow = this.openPreviewWindow.bind(this);
}

Expand Down Expand Up @@ -139,8 +139,8 @@ class AmpPreviewButton extends Component {

if (previewWindow && !previewWindow.closed) {
previewWindow.location = url;
if (this.buttonRef.current) {
this.buttonRef.current.focus();
if (this.itemRef.current) {
this.itemRef.current.focus();
}
}
}
Expand All @@ -159,7 +159,7 @@ class AmpPreviewButton extends Component {
* @param {Event} event The DOM event.
*/
openPreviewWindow(event) {
// Our Preview button has its 'href' and 'target' set correctly for a11y
// Our Preview Menu Item has its 'href' and 'target' set correctly for a11y
// purposes. Unfortunately, though, we can't rely on the default 'click'
// handler since sometimes it incorrectly opens a new tab instead of reusing
// the existing one.
Expand Down Expand Up @@ -208,7 +208,6 @@ class AmpPreviewButton extends Component {
currentPostLink,
errorMessages,
isEnabled,
isSaveable,
isStandardMode,
} = this.props;

Expand All @@ -217,41 +216,44 @@ class AmpPreviewButton extends Component {
// just link to the post's URL.
const href = previewLink || currentPostLink;

if (typeof PluginPreviewMenuItem !== 'function') {
return null;
}

return (
isEnabled &&
!errorMessages.length &&
!isStandardMode && (
<Button
<PluginPreviewMenuItem
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to make sure that if PluginPreviewMenuItem doesn't exist due to not being on the latest version of WordPress that we short-circuit this function so we don't get an error.

className="amp-editor-post-preview"
href={href}
title={__('Preview AMP', 'amp')}
isSecondary
disabled={!isSaveable}
onClick={this.openPreviewWindow}
ref={this.buttonRef}
href={href}
>
<AMPFilledIcon viewBox="0 0 62 62" height={18} width={18} />
{
/* translators: Button label for the AMP preview button */
__('Preview AMP', 'amp')
}
<VisuallyHidden as="span">
{
/* translators: accessibility text */
__('(opens in a new tab)', 'amp')
}
</VisuallyHidden>
</Button>
</PluginPreviewMenuItem>
)
);
}
}

AmpPreviewButton.propTypes = {
AmpPreviewMenuItem.propTypes = {
autosave: PropTypes.func.isRequired,
currentPostLink: PropTypes.string.isRequired,
postId: PropTypes.number.isRequired,
previewLink: PropTypes.string,
isAutosaveable: PropTypes.bool.isRequired,
isDraft: PropTypes.bool.isRequired,
isEnabled: PropTypes.bool.isRequired,
isSaveable: PropTypes.bool.isRequired,
savePost: PropTypes.func.isRequired,
errorMessages: PropTypes.array,
isStandardMode: PropTypes.bool,
Expand Down Expand Up @@ -308,4 +310,4 @@ export default compose([
autosave: dispatch('core/editor').autosave,
savePost: dispatch('core/editor').savePost,
})),
])(AmpPreviewButton);
])(AmpPreviewMenuItem);
15 changes: 15 additions & 0 deletions assets/src/block-editor/plugins/amp-preview-item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* Internal dependencies
*/
import { AMPFilledIcon } from '../../icons';
import AmpPreviewMenuItem from '../components/preview-menu-item';

export const name = 'amp-preview-menu-item';

export const icon = (
<AMPFilledIcon viewBox="0 0 62 62" height={18} width={18} />
Copy link
Member

Choose a reason for hiding this comment

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

The styling of the menu items is a bit different:

image

image

Maybe this is contributing to the icon being cropped at the bottom.

Copy link
Member

Choose a reason for hiding this comment

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

If I make the icon have the 24x24 pixel dimensions then it isn't cropped. But then it's probably too big.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, With the latest build on this PR, it looks like

image

Copy link
Contributor Author

@Takshil-Kunadia Takshil-Kunadia Nov 12, 2024

Choose a reason for hiding this comment

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

Likewise @thelovekesh, The latest build doesn't have the cropping issue on any breakpoints.

Screenshot 2024-11-12 at 3 11 22 PM

Copy link
Member

Choose a reason for hiding this comment

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

Hummm. I still see it cropped with after doing a fresh npm run build:prod:

image

If I increase my browser's zoom level from 100% to 110% then the issue goes away:

image

Maybe it's a ChromeOS-specific rendering issue? In any case, not critical. Would still be good to debug further.

);

export const onlyPaired = true;

export const render = AmpPreviewMenuItem;
68 changes: 0 additions & 68 deletions assets/src/block-editor/plugins/wrapped-amp-preview-button.js

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions assets/src/components/loading/test/__snapshots__/loading.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading