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

[Widgets editor] Load custom block assets #25826

Merged
merged 4 commits into from Oct 7, 2020

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Oct 5, 2020

Description

Fixes #15644 by plugging into should_load_block_editor_scripts_and_styles filter and returning true on widgets editor screen.

Note that this PR will only fix the problem on latest WP trunk, but it won't break anything on older WP versions.

How has this been tested?

  1. Use latest WP trun
  2. Apply this PR
  3. Install and activate, let's say, the CoBlocks plugin
  4. Go to the widgets editor, add a "map" block to any widget area
  5. Confirm it worked

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets labels Oct 5, 2020
@adamziel adamziel self-assigned this Oct 5, 2020
@adamziel adamziel added this to PRs in progress in Block-based Widgets Editor via automation Oct 5, 2020
@adamziel adamziel marked this pull request as ready for review October 5, 2020 11:51
@github-actions
Copy link

github-actions bot commented Oct 5, 2020

Size Change: 0 B

Total Size: 1.18 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 668 B 0 B
build/block-directory/index.js 8.55 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 129 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 8.65 kB 0 B
build/block-library/editor.css 8.65 kB 0 B
build/block-library/index.js 135 kB 0 B
build/block-library/style-rtl.css 7.66 kB 0 B
build/block-library/style.css 7.65 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.6 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.7 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.27 kB 0 B
build/edit-site/index.js 20.5 kB 0 B
build/edit-site/style-rtl.css 3.71 kB 0 B
build/edit-site/style.css 3.72 kB 0 B
build/edit-widgets/index.js 21.4 kB 0 B
build/edit-widgets/style-rtl.css 3 kB 0 B
build/edit-widgets/style.css 3 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

lib/widgets-page.php Outdated Show resolved Hide resolved
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Looks good! Giving you the ✅ so you can merge after addressing mine and Timothy's small comments.

Are we able to use this new filter to simplify any of gutenberg_widgets_init? Or would we have to wait until Gutenberg requires WordPress 5.6?

* @param bool $is_block_editor_screen Current decision about loading block assets.
* @return bool Filtered decision about loading block assets.
*/
function widgets_editor_load_block_editor_scripts_and_styles( $is_block_editor_screen ) {
Copy link
Member

Choose a reason for hiding this comment

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

Convention says this should start with gutenberg_ (e.g. gutenberg_should_load_block_editor_scripts_and_styles) because it's a function added by the Gutenberg plugin.

@adamziel
Copy link
Contributor Author

adamziel commented Oct 6, 2020

Are we able to use this new filter to simplify any of gutenberg_widgets_init? Or would we have to wait until Gutenberg requires WordPress 5.6?

I don't think so @noisysocks, I believe the filter and gutenberg_widgets_init cover two disjoint sets of scripts and styles.

@adamziel adamziel force-pushed the update/widget-editor-load-custom-blocks branch from 89f679f to 479c231 Compare October 7, 2020 10:16
@adamziel
Copy link
Contributor Author

adamziel commented Oct 7, 2020

Merging. Test failures seem to be caused by flakiness - they vary from run to run and seem to be unrelated to this PR.

@adamziel adamziel merged commit 45f1d67 into master Oct 7, 2020
Block-based Widgets Editor automation moved this from PRs in progress to Done Oct 7, 2020
@adamziel adamziel deleted the update/widget-editor-load-custom-blocks branch October 7, 2020 12:12
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 7, 2020
noahtallen added a commit that referenced this pull request Oct 7, 2020
noahtallen added a commit that referenced this pull request Oct 7, 2020
@noahtallen
Copy link
Member

I'm almost positive this PR introduced the test failure related to entry-content because:

  • no commits on master in the past 5 days have failed for that reason
  • every commit on master after this was merged has failed for that reason (until a different PR introduced a new e2e issue, at which point every commit was failing for that reason.)

Also, locally, I get the error related to entry-content in my e2e test runner. But when I revert the merge commit from this PR, the error goes away.

So I did try including a revert of this PR in #25923, and while it made the .entry-content issue go away, I started getting other failures. 🤷 So I haven't done anything about it yet. So I thought I'd comment with my findings so far

@noahtallen
Copy link
Member

I think this fixes it: #25935

@adamziel
Copy link
Contributor Author

adamziel commented Oct 8, 2020

I believe what happened here is that some of the failing tests I've seen were random and varied from rerun to rerun, while others were constant and caused by this PR - I totally missed the latter. When restarting selenium multiple time fixes most of the failures, I find it too easy to dismiss the ones that turn out to be real, especially when the change seems unrelated. While being more careful is the solution for now, I'd love to have these tests based on something more reliable (cypress?) one day. Thank you so much for fixing it @noahtallen ! 🙇

@noahtallen
Copy link
Member

Totally! It also looks like the root issue was in WordPress core, so it was easy to miss locally unless the local environment was completely up to date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Custom Blocks don't show up in Widgets
4 participants