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

Use correct babel-polyfill based on Gutenberg's presence #10969

Merged
merged 13 commits into from
Sep 14, 2018

Conversation

IreneStr
Copy link
Contributor

@IreneStr IreneStr commented Sep 11, 2018

Summary

This PR can be summarized in the following changelog entry:

  • Fixes a bug where a babel-polyfill would throw an error that there shouldn't be two babel-polyfills.

Relevant technical choices:

Test instructions

This PR can be tested by following these steps:

Gutenberg 3.8

  • Download Gutenberg's latest version.
  • Before checking out this wordpress-seo branch, open a post in Gutenberg. You'll see a console warning that there are 2 versions of babel-polyfill.
  • Checkout this wordpress-seo branch. Run composer install, yarn install en yarn start.
  • Open a post in Gutenberg. The error should be gone, and there should not be any other console errors.
  • Also check this without dev-server (remove the variable definition from your wp-config.php and use grunt:build instead of yarn start)
  • Open a post in the classic editor. There should be no console errors.
  • Open a post in the classic editor plugin. There should be no console errors.

Gutenberg pull 9794

  • Checkout the Gutenberg branch from Framework: Add wp-polyfill as central polyfill WordPress/gutenberg#9794. Run composer install, npm install en npm run build.
  • Before checking out this branch, open a post in Gutenberg. You'll see a console warning that there are 2 versions of babel-polyfill.
  • Checkout this branch. Run composer install, yarn install en yarn start.
  • Open a post in Gutenberg. The error should be gone, and there should not be any other console errors.
  • Also check this without dev-server (remove the variable from your wp-config.php and use grunt:build)
  • Open a post in the classic editor. There should be no console errors.
  • Open a post in the classic editor plugin. There should be no console errors.

Quality assurance

  • I have tested this code to the best of my abilities

Fixes #10953

@IreneStr IreneStr changed the base branch from trunk to release/8.3 September 11, 2018 13:48
@abotteram
Copy link
Contributor

CR 👍

@afercia
Copy link
Contributor

afercia commented Sep 12, 2018

Seems the wp-polyfill thing has some upstream issues to start with, reported here: WordPress/gutenberg#9794 (comment)

Regardless, in IE11 I see the error about the double babel polyfill is gone. There are other errors though, and the analysys metabox is completely blank.

@afercia
Copy link
Contributor

afercia commented Sep 12, 2018

Ok so seems the upstream issue has been solved in a separate PR and merged on master. Master wasn't merged in the polyfill PR though. See WordPress/gutenberg#9794 (comment)

@afercia
Copy link
Contributor

afercia commented Sep 12, 2018

After the upstream issue has been solved and after yarn-upgrading yoastseo, most of the errors I was seeing in IE11 are gone.

But...

when activating the Classic Editor plugin, there are a few errors, starting from the edit.php page (the posts list):

screenshot 92

the error points to js/src/wp-seo-quick-edit-handler.js see:
if ( [ "edit.php", "edit-tags.php" ].includes( currentScreen ) ) { ...

so apparently when Gutenberg is disabled, array.includes() is not polyfilled.

Also, there are a few errors in the edit post screen:

screenshot 91

screenshot of the last part:

screenshot 93

seems they're all related to Connect(Metabox) (created by MetaboxPortal) in MetaboxPortal in SlotFillProvider but I have no clue what the actual error is.

@afercia
Copy link
Contributor

afercia commented Sep 13, 2018

P.S. the includes() error on the edit.php page does not happen on trunk.

@afercia
Copy link
Contributor

afercia commented Sep 13, 2018

P.S. 2 also the Connect(Metabox) bla bla error doesn't happen on trunk.

@IreneStr
Copy link
Contributor Author

Acceptance by @dariaknl done 👍
#10953 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants