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

[WIP] Try manipulating the DOM to add the 'Preview AMP' button #3392

Merged

Conversation

kienstra
Copy link
Contributor

preview-amp-button

  • This still needs more testing, but so far, it looks like it's working.
  • It inserts the button into the DOM on domReady, and in that component's componentDidUpdate(), it moves it back to the correct position if needed.
  • Re-rendering of the <Header> had moved the button, as it was inserted in the DOM, but this adds an operation to move it:

generates-amp-preview

This is a possible approach for PR #3323, so it's targeting that branch. For reference, here's full diff against develop.

Related to #2934

This still needs more testing.
But so far, it looks like it's working.
It inserts the button into the DOM on domReady,
and in that component's componentDidUpdate(),
it moves it back to the correct position if needed.
@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 30, 2019
.wp-core-ui .editor-post-preview {
border-top-right-radius: 0;
border-bottom-right-radius: 0;
}
Copy link
Contributor Author

@kienstra kienstra Sep 30, 2019

Choose a reason for hiding this comment

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

@todo: this styling for the non-AMP preview button should only apply if there is a 'Preview AMP' button:

non-amp-preview

@@ -2,7 +2,7 @@
/**
* External dependencies
*/
import { errorMessages, isStandardMode } from 'amp-block-editor-data';
import { errorMessages } from 'amp-block-editor-data';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@todo: there's a console error related to amp-block-editor-data, though probably not caused by this line:

Uncaught ReferenceError: ampBlockEditor is not defined
    at Object.amp-block-editor-data (amp-block-validation.js:5180)
    at __webpack_require__ (amp-block-validation.js:20)
    at Module../assets/src/block-editor/components/amp-preview.js (amp-block-validation.js:114)
    at __webpack_require__ (amp-block-validation.js:20)
    at Module../assets/src/block-editor/components/index.js (amp-block-validation.js:471)
    at __webpack_require__ (amp-block-validation.js:20)
    at Module../assets/src/block-editor/helpers/index.js (amp-block-validation.js:907)
    at __webpack_require__ (amp-block-validation.js:20)
    at Module../assets/src/block-validation/index.js (amp-block-validation.js:2512)
    at __webpack_require__ (amp-block-validation.js:20)

/**
* Moves the (non-AMP) 'Preview' button to before this 'Preview AMP' button, if it's not there already.
*/
moveButton() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this in the 'Preview AMP' button's componentDidUpdate() method moves it back to the correct place.

Without this, here's what it looks like:

amp-render-moved

return;
}

const buttonWrapper = document.createElement( 'div' );
postPreviewButton.parentElement.insertBefore( buttonWrapper, postPreviewButton.nextSibling );
buttonWrapper.id = AMP_PREVIEW_BUTTON_WRAPPER_ID;

render(
<AMPPreview />,
buttonWrapper
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This renders the button into the DOM:

amp-preview-button

@kienstra
Copy link
Contributor Author

What do you think about this DOM rendering?

Hi @swissspidy,
Hope you had a great weekend, and sorry for the delay.

Following up on #2934 (comment), what do you think of this approach of rendering the 'Preview AMP' button into the DOM?

amp-preview-button

It's not ideal, as it could break with changes in Gutenberg. But as far as I've tested it, the button seems to work.

There are some @todos in this PR, it's not ready for a full review.

Thanks, Pascal!

This allows the (non-AMP) preview button
to align with the end of the
.edit-post-sidebar.
@kienstra
Copy link
Contributor Author

kienstra commented Sep 30, 2019

Hm...it looks like the button is unexpectedly moved on first creating a post and editing it. But for other cases, this looks alright.

Update: this should be fixed with 89bc7e8

… post

On creating a new post and making the first edit,
the 'Preview AMP' button appeared out of place.
@kienstra
Copy link
Contributor Author

Question About Overall Approach

Hi @swissspidy,
Hope you're doing great.

What do you think of the overall approach here: rendering the AMP preview button into the DOM with render()?

amp-preview-button

On componentDidUpdate(), it's moved back into position.

This is a little brittle, as it could break with future versions of Gutenberg. But it works so far in my testing.

Thanks, Pascal!

@swissspidy
Copy link
Collaborator

@kienstra The approach is promising and the implementation looks reasonable. In terms of breakage, it doesn't seem to work on WordPress 5.3 RC (either with or without Gutenberg) with all these new styles.

@kienstra
Copy link
Contributor Author

Thanks, Pascal! Good point, I'll look at how it works with WP 5.3 RC.

@kienstra
Copy link
Contributor Author

Merging this try PR into the feature branch, but of course not into develop yet.

@kienstra kienstra merged commit 14df86f into add/block-editor-amp-preview Oct 21, 2019
@kienstra kienstra deleted the try/dom-manipulation-amp-preview branch October 21, 2019 22:41
@westonruter westonruter added this to the v1.5 milestone Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants