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

Only load Gutenberg Polyfill in Gutenberg pages #6849

Merged
merged 1 commit into from May 21, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented May 19, 2018

This PR fixes #6847 by avoiding to load the Gutenberg scripts in the classic editor.
This avoids any TinyMCE conflict.

I also added an e2e test to avoid this kind of regressions in the future.

@youknowriad youknowriad self-assigned this May 19, 2018

@youknowriad youknowriad requested review from pento and WordPress/gutenberg-core May 19, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented May 21, 2018

It seems like a legit fix, however, I'd prefer to have someone else double check PHP logic.

if ( ! gutenberg_can_edit_post( $post ) ) {
return;
}

This comment has been minimized.

@youknowriad

youknowriad May 21, 2018

Contributor

It might be handy to define a is_editor_page function to check whether we're loading a Gutenberg page or not

This comment has been minimized.

@iseulde

iseulde May 21, 2018

Member

Yes, maybe some logic can be shared with pieces of gutenberg_init? Not sure if anything should be different there. It doesn't have the admin and post checks.

This comment has been minimized.

@iseulde

iseulde May 21, 2018

Member

(I don't really understand why it has these checks here, and gutenberg_init does not.)

This comment has been minimized.

@youknowriad

youknowriad May 21, 2018

Contributor

gutenberg/gutenberg.php

Lines 153 to 159 in 70e9e4f

if ( isset( $_GET['classic-editor'] ) ) {
return false;
}
if ( ! gutenberg_can_edit_post( $post ) ) {
return false;
}

@iseulde

Fixes the issue for me. Would be nice to either share the logic with gutenberg_init or explain why different checks are needed. Would also be nice to explain why this is needed: "Ensure the editor module is loaded before third party plugins."

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 21, 2018

"Ensure the editor module is loaded before third party plugins."

This is needed before the previous APIs defined in wp-blocks are loaded in wp-editor and wp-editor is not added as a dependency to these third party plugins. We have a warning showing on the console to help plugin authors migrate but during that period we need to load wp-editor before everything else to avoid breakage.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 21, 2018

gutenberg_init or explain why different checks are needed

The checks are not different. This has an extra check to ensure we're on the editor page get_current_screen. The gutenberg_init is only run in the replace_editor hook or in post.php

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 21, 2018

Merging as this fixed the issue and it's a temporary polyfill anyway.

@youknowriad youknowriad merged commit 6453eb5 into master May 21, 2018

2 checks passed

codecov/project 46.43% remains the same compared to 67bf325
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/polyfill branch May 21, 2018

@mtias mtias added this to the 3.0 milestone Jun 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment