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.

Sign up for GitHub

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

Jump to bottom

Tiled gallery: add wp-polyfill as a dependency #11487

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Tiled gallery: add wp-polyfill as a dependency #11487

merged 1 commit into from
Mar 6, 2019

Conversation

simison
Copy link
Member

@simison simison commented Mar 5, 2019 •
edited
Loading

The previous version of Tiled gallery had wp-i18n as a dependency which depends on wp-polyfill (https://github.com/WordPress/WordPress/blob/029fcf7791e425303d14328471ed61e9f0ecc1e1/wp-includes/script-loader.php#L424).

We removed view side deps in #11326

By adding wp-polyfill, we ensure that isNaN, Array.from and other methods that need polyfilling to work in IE11.

Editor side dependency isn't really needed here since many other dependencies pull in wp-polyfill as well, but seems like it's good to be explicit?

This fix is an alternative to Automattic/wp-calypso#31228 because of size considerations: Automattic/wp-calypso#31228 (comment):

wp-polyfill (v7.0.0) 30.7 KB minified

view.js when building minified production versions from Automattic/wp-calypso#31228:

  • without polyfill 7.96 KiB
  • with polyfill 87.6 KiB

Changes proposed in this Pull Request:

  • Add wp-polyfill as a dependency for tiled gallery block view side and editor side.

Testing instructions:

  • Spin up jurassic ninja with this branch gutenpack-jn
  • Add Tiled gallery to a page where no other blocks are not present (to ensure you're not loading wp-polyfill via other sources)
  • open the saved page in IE11 and observe no more fatal errors in debugger

Fixes Automattic/wp-calypso#31224

Proposed changelog entry for your changes:

Fix regression that caused Tiled Gallery not function in Internet Explorer 11 browsers anymore.

@simison simison added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Pri] High [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 5, 2019
@simison simison added this to the 7.1.1 milestone Mar 5, 2019
@simison simison self-assigned this Mar 5, 2019
@simison simison requested review from a team March 5, 2019 21:11
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D25143-code before merging this PR. Thank you!

This was referenced Mar 5, 2019
Closed
Closed
@jetpackbot
Copy link
Collaborator

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 5a755be

sirreal
sirreal reviewed Mar 5, 2019
View reviewed changes
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I'll give this a test soon, but this looks preferable to Automattic/wp-calypso#31228

@simison
Copy link
Member Author

simison commented Mar 5, 2019 •
edited
Loading

FYI @jeffersonrabb @ockham this might be a good solution for Slideshow block as well to shave some 50KB off from view.js.

@simison
Copy link
Member Author

simison commented Mar 5, 2019

Not in this PR, but just noting down that it would be interesting to try detect old browsers either at the backend or in frontend and load wp-polyfill only when really needed.

@simison simison added the [Status] Needs Review A Jetpack Crew expert's review is needed. Typically for significant changes to core functionality. label Mar 5, 2019
This was referenced Mar 5, 2019
Closed
Closed
@simison
Copy link
Member Author

simison commented Mar 5, 2019

D25143-code deployed.

sirreal
sirreal approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I've tested and this fixes the issue in ie11 for me 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review A Jetpack Crew expert's review is needed. Typically for significant changes to core functionality. labels Mar 6, 2019
jeherve
jeherve approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Looking good on my end. Merging.

@jeherve jeherve merged commit 1d79149 into master Mar 6, 2019
@jeherve jeherve deleted the update/tiled-gallery-polyfill branch March 6, 2019 10:14
@ghost ghost removed the [Status] Needs Changelog label Mar 6, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 6, 2019
jeherve pushed a commit that referenced this pull request Mar 6, 2019
@simison @jeherve
Tiled gallery: add wp-polyfill as a dependency (#11487)
dedbb91 
The previous version of Tiled gallery had `wp-i18n` as a dependency which depends on `wp-polyfill` (https://github.com/WordPress/WordPress/blob/029fcf7791e425303d14328471ed61e9f0ecc1e1/wp-includes/script-loader.php#L424).

We removed view side deps in #11326

By adding `wp-polyfill`, we ensure that `isNaN`, `Array.from` and other methods that need polyfilling to work in IE11.

Editor side dependency isn't _really_ needed here since many other dependencies pull in wp-polyfill as well, but seems like it's good to be explicit?

This fix is an alternative to Automattic/wp-calypso#31228 because of size considerations: Automattic/wp-calypso#31228 (comment):
>wp-polyfill (v7.0.0) 30.7 KB minified
>
>view.js when building minified production versions from Automattic/wp-calypso#31228:
>- without polyfill 7.96 KiB
>- with polyfill 87.6 KiB


#### Changes proposed in this Pull Request:
- Add `wp-polyfill` as a dependency for tiled gallery block view side and editor side.

#### Testing instructions:

- Spin up jurassic ninja with this branch gutenpack-jn
- Add Tiled gallery to a page where no other blocks are not present (to ensure you're not loading `wp-polyfill` via other sources)
- open the saved page in IE11 and observe no more  fatal errors in debugger

Fixes Automattic/wp-calypso#31224

#### Proposed changelog entry for your changes:
Fix regression that caused Tiled Gallery not function in Internet Explorer 11 browsers anymore.
@jeherve
Copy link
Member

jeherve commented Mar 6, 2019

Cherry-picked to branch-7.1 in dedbb91

jeherve added a commit that referenced this pull request Mar 6, 2019
@jeherve
Changelog: add #11487
a64da7e
@sirreal sirreal mentioned this pull request Mar 6, 2019
Merged
jeherve added a commit that referenced this pull request Mar 6, 2019
@jeherve
Changelog: add #11487
08d9292
kraftbj pushed a commit that referenced this pull request Mar 8, 2019
@jeherve @kraftbj
Readme: 7.1.1 updates (#11500)
fd9debd 
* Changelog: add 7.1.1 changelog entries

* Changelog: add #11481

* Changelog: add #11487

* Changelog: add #11489

* Changelog: add #11498
@simison simison mentioned this pull request Mar 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lezama lezama lezama left review comments

@jeherve jeherve jeherve approved these changes

@sirreal sirreal sirreal approved these changes

Assignees

@simison simison

Labels
[Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] High Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
7.1.1
Development

Successfully merging this pull request may close these issues.

Tiled Gallery Block: Galleries not displaying properly in IE11 on simple sites
6 participants
@simison @matticbot @jetpackbot @jeherve @lezama @sirreal